* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Michael Haggerty @ 2009-08-13 22:39 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: Git Mailing List
In-Reply-To: <20090813161256.GA8292@atjola.homenet>
Björn Steinbrink wrote:
> On 2009.08.13 14:46:07 +0200, Michael Haggerty wrote:
>> (Please note that this technique only works for the typical "tracking
>> upstream" type of rebase; it doesn't help with rebases whose goals are
>> changing the order of commits, moving only part of a branch, rewriting
>> commits, etc.)
>
> Hm, so that pretty much doesn't work at all for creating a clean patch
> series, which usually involves rewriting commits, squasing bug fixes
> into the original commits that introduced the bug etc.
Now that you mention it, there are some other uses of rebase whose
history could be recorded correctly, or at least better, in the DAG. I
am not ready to advocate any of these changes, but I think they are
worth discussing.
A squash of two adjacent commits currently transforms this:
A---B1---B2---C
to this
A---B12---C'
(C' has the same contents and commit message as C but a different
history and therefore a different SHA1.)
But B12 includes both B1 and B2, so the correct history is this:
A---B1---B2----C
\ \ \
---------B12---C'
The fact that B12 has B1 and B2 as ancestors tells git that it
incorporates both of their changes, which correctly describes reality.
Splitting a commit (not really an elementary rebase operation, but
achievable with "edit") transforms this:
A---B12---C
to this:
A---B1---B2---C'
It is not possible to represent this correctly in the DAG, because there
is no way to express that "B1" includes part of "B12" as a parent. But
the following would be more accurate than discarding all history:
A---B12---C
\ \ \
B1---B2---C'
It would of course be difficult for the user-interface layer to be
confident that the changes in B1 and B2 are really equivalent to B12
unless the content of B12 and B2 are identical.
Inserting a new commit into the history (for example as part of
reordering later commits) transforms this:
A---C
to this:
A---B---C'
In this case C' includes everything that is in C, so the correct history is
A---C
\ \
B---C'
However, deleting a commit, which transforms this:
A---B---C
to this:
A---C'
cannot be represented in the DAG, because there is no way to express
that C' includes the changes from C without also implying that it
includes the changes from B.
Rewriting a single commit, under the assumption that the rewritten
commit is the logical equivalent of the original, transforms this:
A---B1---C
to this:
A---B2---C'
where B2 is a hand-rewritten version of B1, and C' is the version of C
produced by the rebase. In this case, the history could be recorded as:
A---B1---C
\ \ \
----B2---C'
but again, it is impossible for the user-interface layer to ascertain
that B2 is equivalent to B1 without help from the user.
All of this extra history would currently create far more clutter than
it is worth, but if there were a way to suppress the display of rebased
commits (as discussed in the third article I quoted), then the extra
information would be there to help git without overwhelming users.
> And even for just continously forward porting a series of commits, a
> common case might be that upstream applied some patches, but not all.
> Can you deal with that?
>
> Example:
>
> A---B---C (upstream)
> \
> H---I---J---K (yours)
>
> Upstream takes some changes:
>
> A---B---C---I'--K'--D (upstream)
> \
> H---I---J---K (yours)
>
> rebase leads to:
>
> A---B---C---I'--K'--D (upstream)
> \
> H'--J' (yours)
>
> What would your approach generate in that case?
There *is no way* to represent this history in a DAG, and therefore the
history of this operation will necessarily be lost. (Well, of course it
could be recorded in metadata supplemental to the DAG, but since the
history would not affect future merges it would be pointless.) The
problem is that there is no way to claim that I' is derived from I
without also implying that I' includes the change in H (which it
doesn't). I discuss this sort of thing in another article [1].
>> For more information, please see the full articles: [...]
>
> In this one you have two DAGs:
> (I fixed the second one to also have the merge commit in "subsystem"
> instead of "topic", so they only differ WRT to the rebased stuff)
>
> A)
> m---N---m---m---m---m---m---M (master)
> \ \
> o---o---O---o---o o'--o'--o'--o'--o'--S (subsystem)
> \ /
> *---*---*-..........-*--T (topic)
>
>
> B)
> m---N---m---m---m---m---m---M (master)
> \ \
> \ o'--o'--o'--o'--o'----------S (subsystem)
> \ / / / / / /
> --------------------o---o---O---o---o---*---*---T (topic)
>
>
> And you say that the former creates problems when you want to merge
> again. How so?
As you very clearly showed (thanks!), the merge problems that I claimed
only occur in some obscure edge cases.
What I *should* have emphasized is that the merge S itself is much more
prone to conflicts in case A) (with merge base N) than in case B) (with
the last "o" as merge base). That is the first advantage of
rebase-with-history.
And please note that I really advocate C), not B):
C)
m---N---m---m---m---m---m---M master
\ \
\ o'--o'--o'--o'--o' subsystem
\ / / / / / \
--------------------o---o---O---o---o \
\ \
\ *'--*'--*' topic
\ / / /
*---*---*
where the topic branch is not merged into the subsystem branch but
rather rebased-with-history. C) has the significant advantage over A)
or B) that the topic branch can be converted to a series of patches (the
*' patches) that apply cleanly to the rebased subsystem branch and can
therefore be submitted upstream. In the case of A) or B), the only
available patch that applies cleanly to the rebased subsystem branch is
S, which is a single commit that squashes together the entire topic
branch and is therefore difficult to review.
So rebasing in a public repository makes it difficult for downstream
developers to apply their work to the rebased branch (because they have
to repeat the conflict resolution that was done in the upstream rebase),
and merging in a topic branch makes it more difficult to create an
easily-reviewable patch series. rebase-with-history has neither of
these problems.
Michael
[1] "Git, Mercurial, and Bazaar—simplicity through inflexibility",
http://softwareswirl.blogspot.com/2009/08/git-mercurial-and-bazaarsimplicity.html
^ permalink raw reply
* Re: git cherry to find equivalent commit IDs?
From: skillzero @ 2009-08-13 23:05 UTC (permalink / raw)
To: Avery Pennarun; +Cc: git
In-Reply-To: <32541b130908131437g7cb14a1eh6cd2e31a57f105d5@mail.gmail.com>
On Thu, Aug 13, 2009 at 2:37 PM, Avery Pennarun<apenwarr@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 9:13 PM, <skillzero@gmail.com> wrote:
>> For example, commit 3642151 on branch A was a cherry pick of a commit
>> 460050c on master:
>>
>> $ git branch -a --contains 3642151
>> A
>>
>> $ git branch -a --contains 460050c
>> * master
>>
>> $ git cherry -v master 3642151
>> - 3642151435ce5737debc1213de46dd556475bfad1 fixed bug
>>
>> I assume that means an equivalent change to 3642151 is already in
>> master (which it is, as commit 460050c). But I want to find out the
>> commit ID on master that's equivalent to 3642151 (i.e. something that
>> tells me it's 460050c).
>
> git show 3642151 | git patch-id
>
> You should get a line with two hashes; the first is the patchid (call
> it PATCHID_FROM_ABOVE)
>
> git log -p | git patch-id | grep PATCHID_FROM_ABOVE
>
> This should give you a list of all commits that correspond to that patchid.
That worked perfectly. Thanks.
^ permalink raw reply
* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Björn Steinbrink @ 2009-08-13 23:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Git Mailing List
In-Reply-To: <4A849634.1020609@alum.mit.edu>
On 2009.08.14 00:39:48 +0200, Michael Haggerty wrote:
> Björn Steinbrink wrote:
> > On 2009.08.13 14:46:07 +0200, Michael Haggerty wrote:
> > And even for just continously forward porting a series of commits, a
> > common case might be that upstream applied some patches, but not all.
> > Can you deal with that?
> >
> > Example:
> >
> > A---B---C (upstream)
> > \
> > H---I---J---K (yours)
> >
> > Upstream takes some changes:
> >
> > A---B---C---I'--K'--D (upstream)
> > \
> > H---I---J---K (yours)
> >
> > rebase leads to:
> >
> > A---B---C---I'--K'--D (upstream)
> > \
> > H'--J' (yours)
> >
> > What would your approach generate in that case?
>
> There *is no way* to represent this history in a DAG, and therefore the
> history of this operation will necessarily be lost. (Well, of course it
> could be recorded in metadata supplemental to the DAG, but since the
> history would not affect future merges it would be pointless.) The
> problem is that there is no way to claim that I' is derived from I
> without also implying that I' includes the change in H (which it
> doesn't). I discuss this sort of thing in another article [1].
Well, I' isn't even interesting here. I' is _upstream's_ commit, likely
created from an email. Upstream didn't have "I" when I' was created to
begin with. The interesting commits are H' and J', which were actually
rebased. And the situation is even "worse" than for I' or K'. H' does
include H, I, and K, but not J and J' contains H, I, J and K.
So possibly you'd have:
A---B---C---I'--K'--D (upstream)
\ \
\ --------H'--J'
\ / /
H---I---J---K----
Which only ignores that K is "contained" in H'.
But you only get to know that I is in H' (and that K is in J') _after_
H' (or J') have been created. So you'd need a preprocessing run.
The non-preprocessing result would be something like:
A---B---C---I'--K'--D (upstream)
\ \
\ -------------H'--J'
\/ /
H---I---J--------
\
K
But that's obviously total crap.
> >> For more information, please see the full articles: [...]
> >
> > In this one you have two DAGs:
> > (I fixed the second one to also have the merge commit in "subsystem"
> > instead of "topic", so they only differ WRT to the rebased stuff)
> >
> > A)
> > m---N---m---m---m---m---m---M (master)
> > \ \
> > o---o---O---o---o o'--o'--o'--o'--o'--S (subsystem)
> > \ /
> > *---*---*-..........-*--T (topic)
> >
> >
> > B)
> > m---N---m---m---m---m---m---M (master)
> > \ \
> > \ o'--o'--o'--o'--o'----------S (subsystem)
> > \ / / / / / /
> > --------------------o---o---O---o---o---*---*---T (topic)
> >
> >
> > And you say that the former creates problems when you want to merge
> > again. How so?
>
> As you very clearly showed (thanks!), the merge problems that I claimed
> only occur in some obscure edge cases.
>
> What I *should* have emphasized is that the merge S itself is much more
> prone to conflicts in case A) (with merge base N) than in case B) (with
> the last "o" as merge base). That is the first advantage of
> rebase-with-history.
Yeah, but as I said, actually, topic should have been rebased, not
merged, and then that ends up the same, as you'd do:
git rebase --onto subsystem $last_o topic
Taking the last o commit as upstream.
> And please note that I really advocate C), not B):
>
> C)
> m---N---m---m---m---m---m---M master
> \ \
> \ o'--o'--o'--o'--o' subsystem
> \ / / / / / \
> --------------------o---o---O---o---o \
> \ \
> \ *'--*'--*' topic
> \ / / /
> *---*---*
>
Fine, but this should then be compared to the result from the above
rebase command, which is:
D)
m--...--m (master)
\
o'-...-o' (subsystem)
\
*'-...-*' (topic)
> where the topic branch is not merged into the subsystem branch but
> rather rebased-with-history. C) has the significant advantage over A)
> or B) that the topic branch can be converted to a series of patches (the
> *' patches) that apply cleanly to the rebased subsystem branch and can
> therefore be submitted upstream. In the case of A) or B), the only
> available patch that applies cleanly to the rebased subsystem branch is
> S, which is a single commit that squashes together the entire topic
> branch and is therefore difficult to review.
Same for D)
> So rebasing in a public repository makes it difficult for downstream
> developers to apply their work to the rebased branch (because they have
> to repeat the conflict resolution that was done in the upstream rebase),
There's no difference between C) and D) there, except for the fact that
D) requires you to use --onto, because that needs to differ from
<upstream>.
Let's take the pre-topic-rebase history:
m---m---m (master)
\ \
\ o'--o'--O' (subsystem)
\
o---o---O---*---*---* (topic)
Doing a plain "git rebase subsystem topic" would of course also try to
rebase the "o" commits, so that problematic. Instead, you do:
git rebase --onto subsystem O topic
That turns O..topic (the * commits) into patches, and applies them on
top of O'. So the "o" commits aren't to be rebased.
And that's exactly what your rebase-with-history would do as well. Just
that O is naturally a common ancestor of subsystem and topic, and so
just using "git rebase-w-h subsystem topic" would be enough. Conflicts
etc. should be 100% the same.
If you know that your upstream is going to rebase/rewrite history, you
can tag (or otherwise mark) the current branching point of your branch,
so you can easily specify it for the --onto rebase. IOW: This is
primarily a social problem (tell your downstream that you rebase this or
that branch), but having built-in support to store the branching point
for rebasing _might_ be worth a thought.
> and merging in a topic branch makes it more difficult to create an
> easily-reviewable patch series. rebase-with-history has neither of
> these problems.
Sure, merging is a no-go if you submit patches by email (or other,
similar means). But you compared that to an "enhanced" rebase approach,
instead of comparing your rebase approach to the currently available
one.
So, as I see it, your approach does:
* Save the need to use --onto, allowing to just specify <upstream>, as
if <upstream> was not rewritten.
* Allows to keep older versions of commits more easily accessible for
inspection, e.g. creating interdiffs.
The latter is (to me) of limited use, and the former could be done by
tracking the branching point, not sure how well it work out, but maybe
worth investigating.
Björn
^ permalink raw reply
* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Sitaram Chamarty @ 2009-08-14 3:17 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Björn Steinbrink, Git Mailing List
In-Reply-To: <4A849634.1020609@alum.mit.edu>
Hi,
I'm one of those wannabe experts who thinks he knows enough about git
to teach people in his workplace but obviously pales in this group,
but with that caveat, let me say:
2009/8/14 Michael Haggerty <mhagger@alum.mit.edu>:
> Now that you mention it, there are some other uses of rebase whose
> history could be recorded correctly, or at least better, in the DAG. I
> am not ready to advocate any of these changes, but I think they are
I see you've made your own caveat :-)
> worth discussing.
[snip]
> A---B1---B2----C
> \ \ \
> ---------B12---C'
> A---B12---C
> \ \ \
> B1---B2---C'
[snip]
> A---C
> \ \
> B---C'
[etc etc... many such snipped]
To me, the ability to *forget* the mistakes I made (for whatever
definition of "mistake" you wish) as long as it's private to my repo,
is one of the main attractions of git. I'm one of those guys who
saves early, and saves often, when editing files. This translates to
commit early, commit often, in the git world.
I see no earthly reason why I would ever *want* those commits
preserved, so I hope that, if this sort of thing ever gets into the
code, it is definitely *not* the default :-) It is not sufficient for
me that the GUI knows how to suppress their display, it is necessary
that they *disappear completely*.
And that reminds me. You often hear people on #git ask how to get rid
of some files (maybe containing passwords etc) that inadvertently got
into the repo, and the answer, a lot of the time, is filter-branch,
because the "bad" commit is pretty old. I suspect that for every
person who asks that question on the list because he already pushed,
there are 4 who discovered such an error much earlier, (when the file
went into only a couple of commits at the top maybe), did a rebase -i
with "edit" or whatever, and got rid of the evidence, err I mean
password :-) If this sort of thing were to be the default, they'd
have to use a filter-branch even for such simple cases.
Finally, speaking as someone who teaches git, this adds enormous
complexity to the basic concepts. Complexity is good when the
benefits are obvious, but to me they are not obvious [see *my* caveat
at the top before you react to this statement]
^ permalink raw reply
* [RFC PATCH 0/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List
This is in reply to a brief conversation I had with jnareb on #git
about the blogpost "5 things git could learn from hg", or something
like that (can't find it right now).
Comments most welcome.
Sverre Rabbelier (2):
add a --delete option to git push
test that git push --delete deletes the remote ref
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
t/t5516-fetch-push.sh | 6 ++++++
3 files changed, 22 insertions(+), 2 deletions(-)
^ permalink raw reply
* [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List; +Cc: Sverre Rabbelier
In-Reply-To: <1250226349-20397-1-git-send-email-srabbelier@gmail.com>
Those new to git have a hard time learning how to delete a remote
ref. This makes it easier for them to find out how by providing a
flag to git push.
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
Currently `git push --delete master:master` results in a somewhat
cryptic error message. It seems unlikely however, that those new
to git would use the 'old:new' notation, so I haven't bothered
guarding against it and settled for documenting it.
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 58d2bd5..1ecc6ca 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git push' [--all | --mirror | --tags] [--dry-run] [--receive-pack=<git-receive-pack>]
- [--repo=<repository>] [-f | --force] [-v | --verbose]
+ [--repo=<repository>] [-f | --force] [-v | --verbose] [-d | --delete]
[<repository> <refspec>...]
DESCRIPTION
@@ -137,6 +137,12 @@ useful if you write an alias or script around 'git-push'.
--verbose::
Run verbosely.
+-d::
+--delete::
+ Delete the specified refs. Prefixes all refs with ':' to tell the
+ push machinery to delete the specified ref. As such, the refs
+ that are to be deleted should not contain a ':' specifier.
+
include::urls-remotes.txt[]
OUTPUT
diff --git a/builtin-push.c b/builtin-push.c
index 67f6d96..b954235 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,11 +10,12 @@
#include "parse-options.h"
static const char * const push_usage[] = {
- "git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]",
+ "git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [-d | --delete] [<repository> <refspec>...]",
NULL,
};
static int thin;
+static int push_delete;
static const char *receivepack;
static const char **refspec;
@@ -44,6 +45,12 @@ static void set_refspecs(const char **refs, int nr)
strcat(tag, refs[i]);
ref = tag;
}
+ if (push_delete) {
+ struct strbuf deleted = STRBUF_INIT;
+ strbuf_addstr(&deleted, ":");
+ strbuf_addstr(&deleted, refs[i]);
+ ref = strbuf_detach(&deleted, NULL);
+ }
add_refspec(ref);
}
}
@@ -188,6 +195,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
+ OPT_BOOLEAN('d', "delete", &push_delete, "delete ref"),
OPT_END()
};
--
1.6.4.16.g72c66.dirty
^ permalink raw reply related
* [RFC PATCH 2/2] test that git push --delete deletes the remote ref
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List; +Cc: Sverre Rabbelier
In-Reply-To: <1250226349-20397-2-git-send-email-srabbelier@gmail.com>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
Not a lot of tests yet, but I suspect reviewers will ask for more
and/or new features will pop up that need testing, hence a
seperate patch with tests.
t/t5516-fetch-push.sh | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2d2633f..64da9a6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -586,4 +586,10 @@ test_expect_success 'push with branches containing #' '
git checkout master
'
+test_expect_success 'allow deleting a remote ref with --delete' '
+ mk_test heads/master &&
+ git push --delete testrepo refs/heads/master &&
+ (cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
test_done
--
1.6.4.16.g72c66.dirty
^ permalink raw reply related
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 5:21 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <1250226349-20397-2-git-send-email-srabbelier@gmail.com>
On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
> Those new to git have a hard time learning how to delete a remote
> ref. This makes it easier for them to find out how by providing a
> flag to git push.
Should this also impact refs read from the config? E.g., if I do "git
push --delete" will it try to impact matching refs (which is almost
certainly a mistake), or refs I have in my remote.$X.push (which is
probably also a mistake)?
>From reading your patch, it looks like it just touches the command-line.
I think that's the right thing to do, but I think it makes sense to
think half a second to make sure.
And with the way you have it, "git push --delete" will silently ignore
the --delete and push configured refspecs. Probably it should say
"--delete is useless without refspecs on the command line".
> Currently `git push --delete master:master` results in a somewhat
> cryptic error message. It seems unlikely however, that those new
> to git would use the 'old:new' notation, so I haven't bothered
> guarding against it and settled for documenting it.
It seems like it would be simple enough to just check whether the
refspec contains a colon; if so, silently leave it alone. That could
also protect configured refspecs, as mentioned above, but I wouldn't
rule out somebody have a single-name refspec in their config (in fact, I
think "remote.$X.push = HEAD" is reasonable -- should that delete the
HEAD on "git push --delete"?).
> +--delete::
> + Delete the specified refs. Prefixes all refs with ':' to tell the
> + push machinery to delete the specified ref. As such, the refs
> + that are to be deleted should not contain a ':' specifier.
> +
This impacts _all_ refspecs. Remember that we can have multiple refspecs
on the command-line. So I can "move" a ref remotely with:
git push :old-name old-name:new-name
but I can't do:
git push --delete old-name old-name:new-name
Ignoring colon-less refspecs would make that work, but would not allow
the similar:
# branch has already been renamed to new-name locally
git push --delete old-name new-name
So maybe it would make more sense for it to be "--delete <ref>" and
impact only a single ref. The simple case of "git push --delete foo"
would remain unchanged.
The counter-argument is that "--delete" does not necessarily need to be
as powerful as the ":ref" syntax, but I don't see the downside in making
it so.
-Peff
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Martin Koegler @ 2009-08-14 6:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Frank Lichtenheld, git
In-Reply-To: <7vd46zbjae.fsf@alter.siamese.dyndns.org>
On Thu, Aug 13, 2009 at 12:33:45PM -0700, Junio C Hamano wrote:
> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
> index 557148a..109b7c8 100644
> --- a/builtin-unpack-objects.c
> +++ b/builtin-unpack-objects.c
What about this check:
> @@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)
> if (!obj)
> return 0;
This is neccessary to skip already written objects (eg. blobs,
obj_list[i].obj == NULL). The return code is not important in this
case.
I'm not sure, if fsck_walk can call check_object with obj == NULL
under some (rare) conditions. If yes, the return code should be
changed to 1.
> We probably should write everything as they become unpackable (i.e. as
> their delta bases becomes available) while keeping track of object names
> (but not data) of structured objects that we received, and running only
> one level of reachability check on them at the end. That would certainly
> reduce the memory consumption and may simplify the complexity of the code
> at the same time.
This would defeat the whole idea of the this check: If the
precondition (fsck returns OK for a repository) is met, unpack-objects
(and index-pack) with --strict should gurantee, that this is still
true after receiving the objects (even if somebody intentionally tries
to corrupt the repository).
As we assume, that all objects (and all their ancestors) already
present in the repository are OK, we only have to check new objects
and verify, that linked objects in the repository are of the correct
type.
As soon as we start to write objects without all linked objects
already present in the repository, the repository can get inconsistant.
Lets assume, unpack-objects/receive-pack is changed according to your proposal:
* writeout all objects, as received
* checking reachability of new objects
* update refs, if everything is OK
Then a corruption HOWTO would be:
To introduce a object with one of its linked objects missing, left it
out of the pack and push it into the repository. unpack-objects will
unpack all objects and fail updating the ref (but leave all objects in
the repository). As second step, simply send a ref update request,
which should succed, as the object is present in the repository.
A variant: set the SHA1 of the missing object to the SHA1 of a object
of another type. In the second step, you can sent this object to
unpack-objects, which will unpack it, as it does not know, that the
object is already referenced in the repository.
Deleting objects in the case of an error is also not an option, as a
parallel push operation could have already used the object.
mfg Martin Köger
^ permalink raw reply
* Re: [PATCH] git-cvsimport: add support for cvs pserver password scrambling.
From: Dirk Hörner @ 2009-08-14 6:17 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Johannes Schindelin, Junio C Hamano, Nanako Shiraishi, git
In-Reply-To: <fabb9a1e0908131304m53ab2a68p9faade35969add5@mail.gmail.com>
Hi Sverre,
thanks for the heads up, I will resend it in a minute.
Ciao,
Dirk
2009/8/13 Sverre Rabbelier <srabbelier@gmail.com>:
> Heya,
>
> 2009/8/13 Dirk Hörner <dirker@gmail.com>:
>> sorry for the long delay, but I finally sat down, hacked two testcases
>> and amended the patch after rebasing to the most recent HEAD. Find it
>> attached to this mail.
>
> I think we'd rather find it inlined, as per SubmittingPatches ;).
>
> --
> Cheers,
>
> Sverre Rabbelier
>
^ permalink raw reply
* Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()
From: Karl Wiberg @ 2009-08-14 6:21 UTC (permalink / raw)
To: Catalin Marinas; +Cc: mandolaerik, git
In-Reply-To: <b0943d9e0908131518i3ac18331leb4c0c76313b0780@mail.gmail.com>
On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:
> 2009/8/4 Karl Wiberg <kha@virtutech.com>:
>
> > On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
> >
> > > But can this not lead to a deadlock if the __indata is big? The
> > > stdout of the created process is only processed once the whole
> > > __indata is written. I thought communicate() was created to
> > > avoid this issue.
> >
> > I don't think there's a problem. write() isn't supposed to have a
> > limit on the amount of data it will accept in one call, as far as
> > I'm aware. Plus, it works just fine with Erik's test case---which
> > in my case was about 7 MB. If it can handle 7 MB, I doubt there's
> > a limit we'll hit anytime soon.
>
> write() itself doesn't have a limit, it's mainly what the
> application receiving the data can handle. In the Git case, I think
> it takes all the input as it isn't a filtering tool (things may be
> different with tools like sed etc.).
>
> > Oh, and we still call communicate()---we just don't pass it any
> > additional bytes to write to stdin.
>
> Yes, but if write() is blocked, communicate() won't be called.
Ah, so that's what you were worrying about. Yeah, if the process we're
writing to was going to block us (== not read input) for long periods
of time, we'd have to handle that case. And if it was going to decide
to stop reading input half-way through and give us a sigpipe, we'd
have to handle that. But ...
> Since we are only using Git, I'll merge this patch
Exactly. We really don't expect git to do any of those things to us.
We know it reads all available input before producing the output.
(Well, almost always. I have patches in my experimental branch that
use git cat-file --batch and git diff-tree --stdin, where there can be
many rounds of talking back and forth, but when driving those I'm
being very careful to not deadlock.)
> (and maybe add a comment).
Sure.
--
Karl Wiberg, Virtutech
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 6:24 UTC (permalink / raw)
To: Jeff King; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <20090814052153.GA2881@coredump.intra.peff.net>
Heya,
On Thu, Aug 13, 2009 at 22:21, Jeff King<peff@peff.net> wrote:
> On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
> From reading your patch, it looks like it just touches the command-line.
> I think that's the right thing to do, but I think it makes sense to
> think half a second to make sure.
Indeed, the reason I sent out this RFC was to gather more opinions :).
> And with the way you have it, "git push --delete" will silently ignore
> the --delete and push configured refspecs. Probably it should say
> "--delete is useless without refspecs on the command line".
This makes sense, I will fix that.
>> Currently `git push --delete master:master` results in a somewhat
>> cryptic error message. It seems unlikely however, that those new
>> to git would use the 'old:new' notation, so I haven't bothered
>> guarding against it and settled for documenting it.
>
> It seems like it would be simple enough to just check whether the
> refspec contains a colon; if so, silently leave it alone. That could
> also protect configured refspecs, as mentioned above, but I wouldn't
> rule out somebody have a single-name refspec in their config (in fact, I
> think "remote.$X.push = HEAD" is reasonable -- should that delete the
> HEAD on "git push --delete"?).
I don't think we should touch any configured refspecs, think about how
often one would use that vs. the inconvenience of doing so
unintentionally.
>> +--delete::
>> + Delete the specified refs. Prefixes all refs with ':' to tell the
>> + push machinery to delete the specified ref. As such, the refs
>> + that are to be deleted should not contain a ':' specifier.
>> +
>
> This impacts _all_ refspecs. Remember that we can have multiple refspecs
> on the command-line. So I can "move" a ref remotely with:
Correct, hence the plural 'refs'.
> git push :old-name old-name:new-name
>
> but I can't do:
>
> git push --delete old-name old-name:new-name
I don't think that's the use case for this option, it is mostly for
new users who do not know about the colon notation; now you do raise a
valid point that we might want to add a 'git push --rename old new' at
some point, but I think that's beyond the scope of this patch.
> So maybe it would make more sense for it to be "--delete <ref>" and
> impact only a single ref. The simple case of "git push --delete foo"
> would remain unchanged.
I thought about that, but I decided that it was both intuitive and
convenient to be able to delete multiple refs this way.
> The counter-argument is that "--delete" does not necessarily need to be
> as powerful as the ":ref" syntax, but I don't see the downside in making
> it so.
I do, it's easy to make mistakes when it's more powerful, and I think
less intuitive. I think we want this to be as intuitive as possible.
I'm not very opinionated over any of this, if you have strong feelings
yourself please let me know and I'll change the patch.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Junio C Hamano @ 2009-08-14 6:32 UTC (permalink / raw)
To: Martin Koegler; +Cc: Frank Lichtenheld, git
In-Reply-To: <20090814060307.GA31721@auto.tuwien.ac.at>
Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> To introduce a object with one of its linked objects missing, left it
> out of the pack and push it into the repository. unpack-objects will
> unpack all objects and fail updating the ref (but leave all objects in
> the repository). As second step, simply send a ref update request,
> which should succed, as the object is present in the repository.
Your "ref update request" exploit does not work because your understanding
of how we decide to allow updating a ref is flawed.
We do not blindly update a ref to a commit only because we happen to have
that commit. We require that commit to reach existing tips of refs
without break. The logic is in quickfetch() in builtin-fetch.c.
This stronger validation is necessary to deal with any failed transfer by
http walkers, so it is nothing unusual nor new. They walk from the latest
commits that exist on the other side, and their object transfer can be
interrupted before they transfer enough older commits to make the history
connected with what we already had. In such a case we obviously do not
update any ref. And when we re-run the same request, we do not say "Ah,
we have the tip in the object database, so we will update the ref to it".
That is why I said requiring connectivity is a good idea, but keeping them
in core is a misguided waste of memory.
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 6:33 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <fabb9a1e0908132324td6869aydc752f67b95546f1@mail.gmail.com>
On Thu, Aug 13, 2009 at 11:24:05PM -0700, Sverre Rabbelier wrote:
> > It seems like it would be simple enough to just check whether the
> > refspec contains a colon; if so, silently leave it alone. That could
> > also protect configured refspecs, as mentioned above, but I wouldn't
> > rule out somebody have a single-name refspec in their config (in fact, I
> > think "remote.$X.push = HEAD" is reasonable -- should that delete the
> > HEAD on "git push --delete"?).
>
> I don't think we should touch any configured refspecs, think about how
> often one would use that vs. the inconvenience of doing so
> unintentionally.
I think you are right. My previous message was sort of thinking out
loud, but I think on the whole, the annoyance caused by accidental
deletion is not worth it. :)
> > So maybe it would make more sense for it to be "--delete <ref>" and
> > impact only a single ref. The simple case of "git push --delete foo"
> > would remain unchanged.
>
> I thought about that, but I decided that it was both intuitive and
> convenient to be able to delete multiple refs this way.
> [...]
> I do, it's easy to make mistakes when it's more powerful, and I think
> less intuitive. I think we want this to be as intuitive as possible.
I guess I find what you are doing _more_ complex, because you are really
introducing a whole new mode to push, which is "I am deleting some
stuff". As opposed to some syntactic sugar to replace the confusing
":ref" syntax, which is what I thought the goal was.
On the other hand, "--delete <ref>" introduces its own syntactic
problems. Is it an option, in which case you end up doing:
git push --delete master origin
which is a bit backwards from the usual syntax. Or is it part of the
refspec list, in which case:
1. We have just disallowed a refspec called "--delete" (though to be
fair, you have to be a little insane to use that anyway, and you
can always call it refs/heads/--delete)).
2. Now we don't simply have a list, one refspec per element, which
makes things syntactically a little more complex.
Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
a reasonable way of adding just the syntactic sugar. I.e.:
git push origin --delete=master
Of course, maybe the goal of a "delete mode" is useful to people. I
can't think of a time when I would have used it, but then I also tend to
think ":<ref>" is elegant and obvious. ;)
I dunno. I don't feel too strongly about it; mainly I was just surprised
because I would have done it the other way. :)
-Peff
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 6:40 UTC (permalink / raw)
To: Jeff King; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <20090814063359.GA6898@coredump.intra.peff.net>
Heya,
On Thu, Aug 13, 2009 at 23:33, Jeff King<peff@peff.net> wrote:
> On Thu, Aug 13, 2009 at 11:24:05PM -0700, Sverre Rabbelier wrote:
>> I don't think we should touch any configured refspecs, think about how
>> often one would use that vs. the inconvenience of doing so
>> unintentionally.
>
> I think you are right. My previous message was sort of thinking out
> loud, but I think on the whole, the annoyance caused by accidental
> deletion is not worth it. :)
Thinking out loud is good :).
> I guess I find what you are doing _more_ complex, because you are really
> introducing a whole new mode to push, which is "I am deleting some
> stuff". As opposed to some syntactic sugar to replace the confusing
> ":ref" syntax, which is what I thought the goal was.
Yes, replacing the confusing ":ref" syntax was the goal, but I think
the current solution does that as well.
> On the other hand, "--delete <ref>" introduces its own syntactic
> problems. [...]
It does indeed, and I don't think that's the way to go.
> Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
> a reasonable way of adding just the syntactic sugar. [...]
That would work too I guess, although it would be technically more difficult.
> Of course, maybe the goal of a "delete mode" is useful to people. I
> can't think of a time when I would have used it, but then I also tend to
> think ":<ref>" is elegant and obvious. ;)
I don't think it's that confusing either, but it's hard to stumble
upon, yes? When you're looking at the man page for git push it is
easier to deduct that '--delete' is what you need, than ':master'.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Junio C Hamano @ 2009-08-14 6:53 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Git List
In-Reply-To: <1250226349-20397-2-git-send-email-srabbelier@gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Currently `git push --delete master:master` results in a somewhat
> cryptic error message. ...
What does "git push --delete name:destination" say?
What does "git push --delete :destination" say?
> ... It seems unlikely however, that those new
> to git would use the 'old:new' notation,...
I doubt that assumption is warranted. I've seen new people on this list
who want to be as specific as possible before they get familiar with the
tool (I guess it is in the same spirit that they like to spell out long
option names instead of short ones).
And I happen to think it is a good discipline, when learning a new tool,
to understand the underlying generic model before advancing to lazy and
useful short-hand. That's how I teach in my upcoming book.
Your "old:new" demonstrates a fuzzy understanding of the underlying
concept. They are not <old> nor <new>. They are <object name> and
<destination>; with this object, update that destination. And you can
abbreviate when they are textually spelled the same. I.e. "git push
origin master" is equivalent to "git push origin master:master" because
both sides are spelled 'm a s t e r' the same way.
Having said all that.
I tend to agree with Jeff that it would probably make sense to limit this
new feature to colonless form and error out if you see a refspec with a
colon and --delete at the same time. Also --delete should imply not
looking at configured refspecs at all. After all, this is incompatible
with the way git expresses push with refspecs, and trying to mix these two
would lead to confusion.
I do not mean that this new feature is useless nor stupid. Being able to
say "git push --delete branch1 branch2" matches _a_ mental model (perhaps
Hg inspired one) _very_ naturally. There are branches on the other side,
and there is a special operation called 'delete' that you can inflict on
them.
But it is a different mental model of how git natively does "push". In
git model, you give list of instructions <which branch to update with what
commit>, and as a special case "what commit" could be "empty" to signal
deletion, and "push" carries out the instructions.
These are both valid models. They just do not mix, so let's avoid
confusion by not allowing both at the same time.
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 6:55 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <fabb9a1e0908132340n10da3e38kfab07ab2cff18c82@mail.gmail.com>
On Thu, Aug 13, 2009 at 11:40:44PM -0700, Sverre Rabbelier wrote:
> > On the other hand, "--delete <ref>" introduces its own syntactic
> > problems. [...]
>
> It does indeed, and I don't think that's the way to go.
Hmm. Actually, looking at the code, we _already_ have a funny
two-element syntax:
git push origin tag v1.6.1
which, AFAICT, is totally useless, as you can just push v1.6.1 directly.
I assume it's historical. I still think it's probably not a good idea to
introduce a similar "delete foo".
> > Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
> > a reasonable way of adding just the syntactic sugar. [...]
>
> That would work too I guess, although it would be technically more difficult.
Really? I was thinking something as simple as:
diff --git a/builtin-push.c b/builtin-push.c
index 67f6d96..aa3784c 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -44,6 +44,12 @@ static void set_refspecs(const char **refs, int nr)
strcat(tag, refs[i]);
ref = tag;
}
+ if (!prefixcmp("--delete=", ref)) {
+ struct strbuf deleted = STRBUF_INIT;
+ strbuf_addstr(&deleted, ":");
+ strbuf_addstr(&deleted, skip_prefix(ref, "--delete="));
+ ref = strbuf_detach(&deleted, NULL);
+ }
add_refspec(ref);
}
}
which is even shorter than your patch, not needing a separate option
parser.
That being said, currently parseopt will complain about that, even after
the "remote" field; we would need to pass it
PARSE_OPT_STOP_AT_NON_OPTION.
> I don't think it's that confusing either, but it's hard to stumble
> upon, yes? When you're looking at the man page for git push it is
> easier to deduct that '--delete' is what you need, than ':master'.
I think you mean "deduce", but yes, I think we have seen people complain
about the syntax in the past. I'm not against fixing it; I just want to
make sure what we introduce doesn't make any new confusion.
-Peff
^ permalink raw reply related
* [PATCH v2] git-cvsimport: add support for cvs pserver password scrambling.
From: Dirk Hoerner @ 2009-08-14 6:58 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Junio C Hamano, Nanako Shiraishi,
Dirk Hoerner
Instead of a cleartext password, the CVS pserver expects a scrambled one
in the authentication request. With this patch it is possible to import
CVS repositories only accessible via pserver and user/password.
Signed-off-by: Dirk Hoerner <dirker@gmail.com>
---
git-cvsimport.perl | 39 ++++++++++++++++++++++++++++++++++++++-
t/t9600-cvsimport.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 79 insertions(+), 1 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index e439202..593832d 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -252,7 +252,8 @@ sub conn {
}
};
}
- $pass="A" unless $pass;
+
+ $pass = $self->_scramble($pass);
my ($s, $rep);
if ($proxyhost) {
@@ -484,6 +485,42 @@ sub _fetchfile {
return $res;
}
+sub _scramble {
+ my ($self, $pass) = @_;
+ my $scrambled = "A";
+
+ return $scrambled unless $pass;
+
+ my $pass_len = length($pass);
+ my @pass_arr = split("", $pass);
+ my $i;
+
+ # from cvs/src/scramble.c
+ my @shifts = (
+ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+ 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
+ 114,120, 53, 79, 96,109, 72,108, 70, 64, 76, 67,116, 74, 68, 87,
+ 111, 52, 75,119, 49, 34, 82, 81, 95, 65,112, 86,118,110,122,105,
+ 41, 57, 83, 43, 46,102, 40, 89, 38,103, 45, 50, 42,123, 91, 35,
+ 125, 55, 54, 66,124,126, 59, 47, 92, 71,115, 78, 88,107,106, 56,
+ 36,121,117,104,101,100, 69, 73, 99, 63, 94, 93, 39, 37, 61, 48,
+ 58,113, 32, 90, 44, 98, 60, 51, 33, 97, 62, 77, 84, 80, 85,223,
+ 225,216,187,166,229,189,222,188,141,249,148,200,184,136,248,190,
+ 199,170,181,204,138,232,218,183,255,234,220,247,213,203,226,193,
+ 174,172,228,252,217,201,131,230,197,211,145,238,161,179,160,212,
+ 207,221,254,173,202,146,224,151,140,196,205,130,135,133,143,246,
+ 192,159,244,239,185,168,215,144,139,165,180,157,147,186,214,176,
+ 227,231,219,169,175,156,206,198,129,164,150,210,154,177,134,127,
+ 182,128,158,208,162,132,167,209,149,241,153,251,237,236,171,195,
+ 243,233,253,240,194,250,191,155,142,137,245,235,163,242,178,152
+ );
+
+ for ($i = 0; $i < $pass_len; $i++) {
+ $scrambled .= pack("C", $shifts[ord($pass_arr[$i])]);
+ }
+
+ return $scrambled;
+}
package main;
diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 363345f..57c0eac 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -128,4 +128,45 @@ test_expect_success 'import from a CVS working tree' '
test_expect_success 'test entire HEAD' 'test_cmp_branch_tree master'
+if ! type nc >/dev/null 2>&1
+then
+ say 'skipping cvsimport pserver test, nc not found'
+ test_done
+ exit
+fi
+
+cat << EOF >expected
+BEGIN AUTH REQUEST
+/cvs
+me
+AyuhedEIc?^]'%=0:q Z,b<3!a>
+END AUTH REQUEST
+EOF
+
+test_expect_success 'connect to pserver with password' '
+
+ echo "I HATE YOU" | nc -l 2401 >actual &
+ test_must_fail git cvsimport -d \
+ :pserver:me:abcdefghijklmnopqrstuvwxyz@localhost:/cvs foo \
+ >/dev/null 2>&1 &&
+ test_cmp expected actual
+'
+
+cat << EOF >expected
+BEGIN AUTH REQUEST
+/cvs
+anonymous
+A
+END AUTH REQUEST
+EOF
+
+test_expect_success 'connect to pserver without password' '
+
+ echo "I HATE YOU" | nc -l 2401 >actual &
+ test_must_fail git cvsimport -d \
+ :pserver:anonymous@localhost:/cvs foo \
+ >/dev/null 2>&1 &&
+ test_cmp expected actual
+'
+
test_done
--
1.6.4
^ permalink raw reply related
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 7:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, Jakub Narebski, Git List
In-Reply-To: <7vab22ubr5.fsf@alter.siamese.dyndns.org>
On Thu, Aug 13, 2009 at 11:53:50PM -0700, Junio C Hamano wrote:
> I do not mean that this new feature is useless nor stupid. Being able to
> say "git push --delete branch1 branch2" matches _a_ mental model (perhaps
> Hg inspired one) _very_ naturally. There are branches on the other side,
> and there is a special operation called 'delete' that you can inflict on
> them.
>
> But it is a different mental model of how git natively does "push". In
> git model, you give list of instructions <which branch to update with what
> commit>, and as a special case "what commit" could be "empty" to signal
> deletion, and "push" carries out the instructions.
>
> These are both valid models. They just do not mix, so let's avoid
> confusion by not allowing both at the same time.
Thank you for succintly explaining what I was trying to put my finger on
elsewhere in the thread. To sum up what I was trying to say:
I don't have anything against the "delete mode" mental model, but I
don't have a particular use for it. My counter-proposal was "syntactic
sugar without changing the mental model". Between the two, I don't have
a strong feeling (and my "this is wrong" comments were about where you
had mixed the two, and I think you agreed with them).
-Peff
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 7:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sverre Rabbelier, Jakub Narebski, Git List
In-Reply-To: <20090814070021.GB7113@coredump.intra.peff.net>
On Fri, Aug 14, 2009 at 03:00:21AM -0400, Jeff King wrote:
> Thank you for succintly explaining what I was trying to put my finger on
> elsewhere in the thread. To sum up what I was trying to say:
>
> I don't have anything against the "delete mode" mental model, but I
> don't have a particular use for it. My counter-proposal was "syntactic
> sugar without changing the mental model". Between the two, I don't have
> a strong feeling (and my "this is wrong" comments were about where you
> had mixed the two, and I think you agreed with them).
To clarify: "you" in the first paragraph is Junio, and in the second it
is Sverre. :)
-Peff
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 7:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, Git List
In-Reply-To: <7vab22ubr5.fsf@alter.siamese.dyndns.org>
Heya,
On Thu, Aug 13, 2009 at 23:53, Junio C Hamano<gitster@pobox.com> wrote:
> I doubt that assumption is warranted. I've seen new people on this list
> who want to be as specific as possible before they get familiar with the
> tool (I guess it is in the same spirit that they like to spell out long
> option names instead of short ones).
Fair enough.
> Your "old:new" demonstrates a fuzzy understanding of the underlying
> concept. They are not <old> nor <new>. They are <object name> and
> <destination>; with this object, update that destination.
Not really, I was responding to Jeff's "you can rename a branch like
this", I know that in <left>:<right> the <left> can be anything that
locally resolves to an object, and that <right> can be any valid ref
on the remote side, but that's quite a mouthful :P.
> Also --delete should imply not
> looking at configured refspecs at all. After all, this is incompatible
> with the way git expresses push with refspecs, and trying to mix these two
> would lead to confusion.
Agreed, I don't think "git push --delete origin master:master
to-be-deleted" is a good idea.
> [...] Being able to
> say "git push --delete branch1 branch2" matches _a_ mental model (perhaps
> Hg inspired one) _very_ naturally. [...]
>
> [...] In
> git model, you give list of instructions <which branch to update with what
> commit>, and as a special case "what commit" could be "empty" to signal
> deletion, and "push" carries out the instructions.
>
> These are both valid models. They just do not mix, so let's avoid
> confusion by not allowing both at the same time.
Agreed, would it be enough to ensure that there are refs present (as
argument), and
that they do not contain a colon?
Also, how do I go about making sure the local configuration is
ignored, as you mentioned above?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Martin Koegler @ 2009-08-14 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Frank Lichtenheld, git
In-Reply-To: <7vocqiucpw.fsf@alter.siamese.dyndns.org>
On Thu, Aug 13, 2009 at 11:32:59PM -0700, Junio C Hamano wrote:
> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> > To introduce a object with one of its linked objects missing, left it
> > out of the pack and push it into the repository. unpack-objects will
> > unpack all objects and fail updating the ref (but leave all objects in
> > the repository). As second step, simply send a ref update request,
> > which should succed, as the object is present in the repository.
>
> Your "ref update request" exploit does not work because your understanding
> of how we decide to allow updating a ref is flawed.
>
> We do not blindly update a ref to a commit only because we happen to have
> that commit. We require that commit to reach existing tips of refs
> without break. The logic is in quickfetch() in builtin-fetch.c.
I'm talking on the server side of a push operation (receive-pack), not
the client side. The patchset should prevent invalid data from
entering the repository, thereby preventing upload-pack (during further
fetch operation) and other git programs (eg. called from gitweb) from
failing/segfaulting.
mfg Martin Kögler
^ permalink raw reply
* Re: [PATCH v2] git-cvsimport: add support for cvs pserver password scrambling.
From: Junio C Hamano @ 2009-08-14 7:25 UTC (permalink / raw)
To: Dirk Hoerner; +Cc: git, Johannes Schindelin, Nanako Shiraishi
In-Reply-To: <1250233111-19188-1-git-send-email-dirker@gmail.com>
Dirk Hoerner <dirker@gmail.com> writes:
> Instead of a cleartext password, the CVS pserver expects a scrambled one
> in the authentication request. With this patch it is possible to import
> CVS repositories only accessible via pserver and user/password.
>
> Signed-off-by: Dirk Hoerner <dirker@gmail.com>
Thanks.
While I appreciate your effort to add a test, I'd rather not apply the
test part of your patch for two reasons:
- It is not a test against a real cvs pserver but is a whitebox test to
verify that the program says what the program is supposed to spit out
to the network; and
- It still is a network test that will fail if the TCP port is occupied
for whatever reason when the test is run, which will make automated
build and test cycle unreliable.
Unfortunately, I do not see an easy way to run a real cvs pserver
listening to a local unix domain socket under $TRASH_DIRECTORY, which
would solve both of the above issues.
^ permalink raw reply
* Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()
From: Erik Sandberg @ 2009-08-14 7:26 UTC (permalink / raw)
To: Karl Wiberg; +Cc: Catalin Marinas, git
In-Reply-To: <20090814062149.GA23179@lux.e.vtech>
On Fri, Aug 14, 2009 at 8:21 AM, Karl Wiberg<kha@virtutech.com> wrote:
> On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:
>
>> Since we are only using Git, I'll merge this patch
>> (and maybe add a comment).
>
> Sure.
Wouldn't it be better to only use the write() workaround when
sys.version indicates that the Python bug may be present, and use
communicate() properly otherwise?
Erik
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Junio C Hamano @ 2009-08-14 7:31 UTC (permalink / raw)
To: Martin Koegler; +Cc: Frank Lichtenheld, git
In-Reply-To: <20090814071949.GA2342@auto.tuwien.ac.at>
Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> I'm talking on the server side of a push operation (receive-pack), not
> the client side. The patchset should prevent invalid data from
> entering the repository, thereby preventing upload-pack (during further
> fetch operation) and other git programs (eg. called from gitweb) from
> failing/segfaulting.
upload-pack won't feed starting from a random object for a reason, so your
worry is unfounded.
I'd agree gitweb may be a problem. Ideally it should restrict itself from
anything reachable from the refs of he repository---patches welcome.
^ permalink raw reply
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