* [PATCH] send-pack: Filter unknown commits from alternates of the remote
From: Björn Steinbrink @ 2009-01-28 1:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: PJ Hyett, Shawn O. Pearce, Johannes Schindelin, Linus Torvalds,
git
In-Reply-To: <7vk58gz04l.fsf@gitster.siamese.dyndns.org>
Since 40c155ff14c, receive-pack on the remote also sends refs from its
alternates. Unfortunately, we don't filter commits that don't exist in the
local repository from that list. This made us pass those unknown commits
to pack-objects, causing it to fail with a "bad object" error.
Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
builtin-send-pack.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9..10d7016 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
* parameters by writing to the pipe.
*/
for (i = 0; i < extra->nr; i++) {
- memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
- buf[0] = '^';
- buf[41] = '\n';
- if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
- break;
+ if (!is_null_sha1(&extra->array[i][0]) &&
+ has_sha1_file(&extra->array[i][0])) {
+ memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
+ buf[0] = '^';
+ buf[41] = '\n';
+ if (!write_or_whine(po.in, buf, 42,
+ "send-pack: send refs"))
+ break;
+ }
}
while (refs) {
--
1.6.1.284.g5dc13.dirty
^ permalink raw reply related
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Junio C Hamano @ 2009-01-28 1:44 UTC (permalink / raw)
To: Shawn O. Pearce, Johannes Schindelin; +Cc: git, PJ Hyett
In-Reply-To: <7vk58gz04l.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The extra "we also have these" advertisement happened as a result of this
> discussion:
>
> http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256
>
> I think I know what is going on.
>
> Consider this sequence of events.
>
> (0) Alice creates a project and pushes to public.
>
> alice$ cd $HOME/existing-tarball-extract
> alice$ git init
> alice$ git add .
> alice$ git push /pub/alice.git master
>
>
> (1) Bob forks it.
>
> bob$ git clone --bare --reference /pub/alice.git /pub/bob.git
I need another /pub/alice.git here, I think, but I hope I got the point
across to people who are capable of helping us to resolve this issue.
>
> (2) Bob clones his.
>
> bob$ cd $HOME && git clone /pub/bob.git bob
>
> (3) Alice works more and pushes
>
> alice$ edit foo
> alice$ git add foo
> alice$ git commit -a -m 'more'
> alice$ git push /pub/alice.git master
>
> (4) Bob works more and tries to push to his.
>
> bob$ cd $HOME/bob
> bob$ edit bar
> bob$ git add bar
> bob$ git commit -a -m 'yet more'
> bob$ git push /pub/bob.git master
>
> Now, the new server advertises the objects reachable from alice's branch
> tips as usable cut-off points for pack-objects bob will run when sending.
>
> And new builtin-send-pack.c has new code that feeds "extra" refs as
>
> ^SHA1\n
>
> to the pack-objects process.
>
> The latest commit Alice created and pushed into her repository is one such
> commit.
>
> But the problem is that Bob does *NOT* have it. His "push" will run pack
> object telling it that objects reachable from Alice's top commit do not
> have to be sent, which was the whole point of doing this new "we also have
> these" advertisement, but instead of ignoring that unknown commit,
> pack-objects would say "Huh? I do not even know that commit" and dies.
>
> This can and should be solved by client updates, as 1.6.1 server can work
> with older client just fine.
Here is a *wrong* fix that should work most of the time. It will
certainly appear to fix the issue in the above reproduction recipe.
You may want to ask your users to try this to see if it makes their
symptom disappear.
When we receive ".have" advertisement, this wrong fix checks if that
object is available locally, and it ignores it otherwise.
This won't be acceptable as the official fix. We should be doing the
full connectivity check; in other words, not just "do we have it", but "do
we have it *and* is it reachable from any of our own refs".
connect.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git c/connect.c w/connect.c
index 2f23ab3..8026850 100644
--- c/connect.c
+++ w/connect.c
@@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
{
+ if (!has_sha1_file(sha1))
+ return;
+
ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
hashcpy(&(extra->array[extra->nr][0]), sha1);
extra->nr++;
^ permalink raw reply related
* Re: [PATCHv4] gitweb: make static files accessible with PATH_INFO
From: Jakub Narebski @ 2009-01-28 1:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Giuseppe Bilotta, git
In-Reply-To: <7vocxsz19e.fsf@gitster.siamese.dyndns.org>
On Wed, 28 Jan 2009, Junio C Hamano wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
> > When PATH_INFO is defined,
I other words: when using PATH_INFO gitweb URL
> > static files such as the default CSS or the
> > shortcut icon are not accessible beyond the summary page (e.g. in
> > shortlog or commit view).
To be more exact: if static files, such as the default CSS (and also
other stylesheets, if defined), the gitweb favicon / shortcut icon,
or the gitweb logo locations (URIs) are defined using relative URL
with relative path (and default Makefile rule to generate gitweb.cgi
uses basenames like 'gitweb.css', which are relative paths) _THEN_
those static files are not accessible beyond 'summary' and projects
list view (e.g. in 'shortlog' or 'commit' view).
> >
> > Fix this by adding a <base> tag pointing to the script's own URL.
Which defines base URL for all relative links in gitweb.
/Note that this makes the document hierarchy non-portable, but I don't
think we care.../
> >
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> > ---
> > gitweb/gitweb.perl | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 931db4f..55e3081 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2901,6 +2901,11 @@ sub git_header_html {
> > <meta name="robots" content="index, nofollow"/>
> > <title>$title</title>
> > EOF
> > +# the stylesheet, favicon etc urls won't work correctly with path_info unless we
> > +# set the appropriate base URL
+# the stylesheet, favicon, etc URLs won't work correctly with path_info,
+# unless we set the appropriate base URL
By the way, I think those comments should be indented aligned to
accompanying code, but this is issue of separate whitespace cleanup,
independent of the issue this commit tries to solve.
> > + if ($ENV{'PATH_INFO'}) {
> > + print "<base href=\"$my_url\" />\n";
> > + }
>
> Perhaps this is a stupid question, but is $my_url already safe to include
> in the output without any further quoting at this point in the codepath?
Hmmmm... to be safe we probably should use either esc_url($my_url) here
(like in git_feed subroutine).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote
From: Junio C Hamano @ 2009-01-28 1:47 UTC (permalink / raw)
To: Björn Steinbrink
Cc: PJ Hyett, Shawn O. Pearce, Johannes Schindelin, Linus Torvalds,
git
In-Reply-To: <20090128013840.GA7224@atjola.homenet>
Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> Since 40c155ff14c, receive-pack on the remote also sends refs from its
> alternates. Unfortunately, we don't filter commits that don't exist in the
> local repository from that list. This made us pass those unknown commits
> to pack-objects, causing it to fail with a "bad object" error.
Yeah, it is a step in the right direction, but is the *wrong* fix I
described in my previous message.
Our mails crossed ;-)
And I think we should have this in the place where we receive .have,
i.e. inside add_extra_have() in connect.c
^ permalink raw reply
* Re: Heads up: rebase -i -p will be made sane again
From: Johannes Schindelin @ 2009-01-28 1:53 UTC (permalink / raw)
To: Stephen Haberman; +Cc: git, gitster
In-Reply-To: <20090127085418.e113ad5a.stephen@exigencecorp.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2797 bytes --]
Hi,
On Tue, 27 Jan 2009, Stephen Haberman wrote:
>
> Dscho wroteÖ
>
> > As for the design bug I want to fix: imagine this history:
> >
> > ------A
> > / /
> > / /
> > ---- B
> > \ \
> > \ \
> > C-----D-----E = HEAD
> >
> > A, C and D touch the same file, and A and D agree on the contents.
> >
> > Now, rebase -p A does the following at the moment:
> >
> > ------A-----E' = HEAD
> > / /
> > / /
> > ---- B
> >
> > In other words, C is truly forgotten, and it is pretended that D never
> > happened, either. That is exactly what test case 2 in t3410 tests for
> > [*1*].
> >
> > This is insane.
>
> Agreed.
Actually, I misread t3410 a great deal. The situation is as follows:
... UPSTREAM
\
... A - B - C -D
A is a patch the upstream does not have, B is a patch UPSTREAM has,
and "git diff C^!" (i.e. the diff of C to its first parent) is _also_
identical to a diff of a merge that is in UPSTREAM.
Basically, t3410 tests that after "git rebase -i -p UPSTREAM" and leaving
the rebase script as-is, essentially, A and D are cherry-picked on top of
UPSTREAM.
> Does this mean you're just getting rid of the code that calls "rev list
> --cherry-pick"?
Only now do I understand.
I misread the code for --cherry-pick. For merges, it adds the diff to the
first parent!
I do not know if it really is desirable to have --cherry-pick handle
merges at all; I tend to think it is not.
Unfortunately, a short blame session just points to 9c6efa36 done by a
sloppy programmer: yours truly.
So I adapted my code to find the "dropped" merges in
git-rebase--interactive, too, for now, but I guess the proper fix is
something like this:
-- snipsnap --
[PATCH] --cherry-pick: do not skip merges, ever
Currently, --cherry-pick has no problem getting a patch id for merge
commits: it calculated as the patch id of the patch between the first
parent and the merge commit.
Of course, this is bogus, as it completely misses the fact that the
merge commit has other parents, too, and therefore a single patch id
would be wrong.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
patch-ids.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..808a7f0 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -6,9 +6,12 @@
static int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1)
{
- if (commit->parents)
+ if (commit->parents) {
+ if (commit->parents->next)
+ return 0; /* merges do not have a patch id */
diff_tree_sha1(commit->parents->item->object.sha1,
commit->object.sha1, "", options);
+ }
else
diff_root_tree_sha1(commit->object.sha1, "", options);
diffcore_std(options);
^ permalink raw reply related
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: PJ Hyett @ 2009-01-28 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, Johannes Schindelin, git
In-Reply-To: <7vfxj4yzjj.fsf@gitster.siamese.dyndns.org>
On Tue, Jan 27, 2009 at 5:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The extra "we also have these" advertisement happened as a result of this
>> discussion:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/95072/focus=95256
>>
>> I think I know what is going on.
>>
>> Consider this sequence of events.
>>
>> (0) Alice creates a project and pushes to public.
>>
>> alice$ cd $HOME/existing-tarball-extract
>> alice$ git init
>> alice$ git add .
>> alice$ git push /pub/alice.git master
>>
>>
>> (1) Bob forks it.
>>
>> bob$ git clone --bare --reference /pub/alice.git /pub/bob.git
>
> I need another /pub/alice.git here, I think, but I hope I got the point
> across to people who are capable of helping us to resolve this issue.
>
>>
>> (2) Bob clones his.
>>
>> bob$ cd $HOME && git clone /pub/bob.git bob
>>
>> (3) Alice works more and pushes
>>
>> alice$ edit foo
>> alice$ git add foo
>> alice$ git commit -a -m 'more'
>> alice$ git push /pub/alice.git master
>>
>> (4) Bob works more and tries to push to his.
>>
>> bob$ cd $HOME/bob
>> bob$ edit bar
>> bob$ git add bar
>> bob$ git commit -a -m 'yet more'
>> bob$ git push /pub/bob.git master
>>
>> Now, the new server advertises the objects reachable from alice's branch
>> tips as usable cut-off points for pack-objects bob will run when sending.
>>
>> And new builtin-send-pack.c has new code that feeds "extra" refs as
>>
>> ^SHA1\n
>>
>> to the pack-objects process.
>>
>> The latest commit Alice created and pushed into her repository is one such
>> commit.
>>
>> But the problem is that Bob does *NOT* have it. His "push" will run pack
>> object telling it that objects reachable from Alice's top commit do not
>> have to be sent, which was the whole point of doing this new "we also have
>> these" advertisement, but instead of ignoring that unknown commit,
>> pack-objects would say "Huh? I do not even know that commit" and dies.
>>
>> This can and should be solved by client updates, as 1.6.1 server can work
>> with older client just fine.
>
> Here is a *wrong* fix that should work most of the time. It will
> certainly appear to fix the issue in the above reproduction recipe.
> You may want to ask your users to try this to see if it makes their
> symptom disappear.
>
> When we receive ".have" advertisement, this wrong fix checks if that
> object is available locally, and it ignores it otherwise.
>
> This won't be acceptable as the official fix. We should be doing the
> full connectivity check; in other words, not just "do we have it", but "do
> we have it *and* is it reachable from any of our own refs".
>
> connect.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git c/connect.c w/connect.c
> index 2f23ab3..8026850 100644
> --- c/connect.c
> +++ w/connect.c
> @@ -43,6 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
>
> static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1)
> {
> + if (!has_sha1_file(sha1))
> + return;
> +
> ALLOC_GROW(extra->array, extra->nr + 1, extra->alloc);
> hashcpy(&(extra->array[extra->nr][0]), sha1);
> extra->nr++;
>
Thank you for your detailed response. To answer your previous
question, all of the bug reports have been made by users running
1.6.1.
My concern is that we obviously have no control over what version of
Git our 50k+ users are running, and we will be perpetually stuck
running 1.5 on the servers to account for this issue.
Is there any possibility to have the server code in an upcoming
release account for clients running 1.6.1?
-PJ
^ permalink raw reply
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Shawn O. Pearce @ 2009-01-28 2:02 UTC (permalink / raw)
To: PJ Hyett; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <bab6a2ab0901271757i4602774ahef1d881b7ed58097@mail.gmail.com>
PJ Hyett <pjhyett@gmail.com> wrote:
>
> Is there any possibility to have the server code in an upcoming
> release account for clients running 1.6.1?
I can't think off-hand of a way for the server to know what version
the client is. There's nothing really different in the protocol
between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
include-tag) or later client.
So there's no easy way for the server to work around this possible
glitch in the client.
--
Shawn.
^ permalink raw reply
* Re: friendlier names
From: Jakub Narebski @ 2009-01-28 2:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, David Abrahams, git
In-Reply-To: <7vocxsy1dd.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > It is a bit of pity that "git add" was overloaded to also add new
> > contents and not only add new file (and its contents!), instead of
> > having new command "git stage" to be porcelain version of
> > "git update-index" porcelain. And perhaps "git resolved" to only
> > mark resolved entries (so e.g. "git resolved ." would not add new
> > files, nor add new contents of files which were not in conflict).
>
> I do not think so.
>
> People who are taught with various means (including "git stage" alias)
> understand that you prepare the contents you want to record in the commit
> you are about to make by updating the contents registered in the index aka
> staging area, then you do not need "git resolved".
"Who are taught". This makes for Git to be more "user selective".
Not that this matter much, as world domination is not our goal ;-)
> You resolve, you have the desired content in your work tree, and you
> register the updated contents from your work tree to the index aka staging
> area, in exactly the same way as you do when you want to include updated
> contents for any commit.
While I don't think that "git resolved" is something really needed,
the difference is with "git add ." and "git resolved ." and between
"git add *" and "git resolved *", where the latter would update only
resolved merge conflict resolutions, and would not pick up independent
changes to the files that were not in conflict.
BTW with "git add" way you have to know that "git add"-ing a file
would clear 'is in merge conflict' flags (well, will hide >0 stages...).
--
Jakub Narebski
Poland
^ permalink raw reply
* [PATCH 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin
From: Jay Soffian @ 2009-01-28 2:38 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, gitster, sverre
When git am is called w/o arguments, w/o a patch on stdin and the user hits
ctrl-c, it leaves behind a partially populated $dotest directory. After this
commit, it emits usage when called w/o arguments and w/o a patch on stdin.
Also ensure that $dotest is cleaned up if user manages to interupt mailsplit
while it is processing input.
Noticed by Sverre Rabbelier
---
git-am.sh | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index b1c05c9..36227c6 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -254,10 +254,10 @@ else
done
shift
fi
- git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" || {
- rm -fr "$dotest"
- exit 1
- }
+ test $# = 0 && test -t 0 && usage
+ trap 'rm -fr "$dotest"' 0
+ git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" || exit 1
+ trap - 0
# -s, -u, -k, --whitespace, -3, -C and -p flags are kept
# for the resuming session after a patch failure.
--
1.6.1.224.gb56c
^ permalink raw reply related
* [PATCH 2/2] git-am: minor cleanups
From: Jay Soffian @ 2009-01-28 2:38 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, gitster, sverre
In-Reply-To: <1233110338-16806-1-git-send-email-jaysoffian@gmail.com>
Update usage statement to remove a no-longer supported option, and to hide two
options (one a no-op, one internal) unless --help-all is used.
Use "test -t 0" instead of deprecated "tty -s" to detect when stdin is a
terminal. (test -t 0 is used elsewhere in git-am and in other git shell
scripts, tty -s is not.)
Use "test ..." instead of "[ ... ]" and "die <msg>" instead of "echo <msg>
>&2; exit 1" to be consistent with rest of script.
---
While fixing Sverre's issue, I noticed a few other things about git-am that
seemed worth cleaning up. These seem much to small to split into separate
patches so I included them together. Let me know if they should be split.
git-am.sh | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index 36227c6..2ad229b 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -8,9 +8,8 @@ OPTIONS_SPEC="\
git am [options] [<mbox>|<Maildir>...]
git am [options] (--resolved | --skip | --abort)
--
-d,dotest= (removed -- do not use)
i,interactive run interactively
-b,binary (historical option -- no-op)
+b,binary* (historical option -- no-op)
3,3way allow fall back on 3way merging if needed
s,signoff add a Signed-off-by line to the commit message
u,utf8 recode into utf8 (default)
@@ -24,7 +23,7 @@ resolvemsg= override error message when patch failure occurs
r,resolved to be used after a patch failure
skip skip the current patch
abort restore the original branch and abort the patching operation.
-rebasing (internal use for git-rebase)"
+rebasing* (internal use for git-rebase)"
. git-sh-setup
prefix=$(git rev-parse --show-prefix)
@@ -204,7 +203,7 @@ then
# unreliable -- stdin could be /dev/null for example
# and the caller did not intend to feed us a patch but
# wanted to continue unattended.
- tty -s
+ test -t 0
;;
*)
false
@@ -280,10 +279,7 @@ fi
case "$resolved" in
'')
files=$(git diff-index --cached --name-only HEAD --) || exit
- if [ "$files" ]; then
- echo "Dirty index: cannot apply patches (dirty: $files)" >&2
- exit 1
- fi
+ test "$files" && die "Dirty index: cannot apply patches (dirty: $files)"
esac
if test "$(cat "$dotest/utf8")" = t
--
1.6.1.224.gb56c
^ permalink raw reply related
* Re: [PATCH 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin
From: Jay Soffian @ 2009-01-28 2:53 UTC (permalink / raw)
To: git; +Cc: gitster, sverre
In-Reply-To: <1233110338-16806-1-git-send-email-jaysoffian@gmail.com>
On Tue, Jan 27, 2009 at 9:38 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> When git am is called w/o arguments, w/o a patch on stdin and the user hits
> ctrl-c, it leaves behind a partially populated $dotest directory. After this
> commit, it emits usage when called w/o arguments and w/o a patch on stdin.
>
> Also ensure that $dotest is cleaned up if user manages to interupt mailsplit
> while it is processing input.
>
> Noticed by Sverre Rabbelier
Ugh, I forgot to sign-off on this and the next patch. Should I resend
or is it okay to say:
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
j.
^ permalink raw reply
* Re: [PATCH 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin
From: Junio C Hamano @ 2009-01-28 3:01 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, gitster, sverre
In-Reply-To: <1233110338-16806-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> Also ensure that $dotest is cleaned up if user manages to interupt mailsplit
> while it is processing input.
I do not think this "Also ensure" part is necessary nor desirable. If
something goes wrong, we'd like to have a way to resurrect some partial
results from the split out result.
The rest looked fine. Perhaps a re-roll?
^ permalink raw reply
* Re: [PATCH 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin
From: Jay Soffian @ 2009-01-28 3:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sverre
In-Reply-To: <7v7i4gyvzr.fsf@gitster.siamese.dyndns.org>
On Tue, Jan 27, 2009 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> Also ensure that $dotest is cleaned up if user manages to interupt mailsplit
>> while it is processing input.
>
> I do not think this "Also ensure" part is necessary nor desirable. If
> something goes wrong, we'd like to have a way to resurrect some partial
> results from the split out result.
Hmm, git-am was already nuking $dotest if mailsplit exited non-zero,
so I kept that behavior, but also clean-up if user kills git-am while
mailsplit is running.
I still think it should nuke $dotest if user hits ctrl-c, but I agree
if mailsplit exits non-zero it should not (which would be a change
from what it does now).
> The rest looked fine. Perhaps a re-roll?
Sure.
j.
^ permalink raw reply
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Junio C Hamano @ 2009-01-28 3:09 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: PJ Hyett, Johannes Schindelin, git
In-Reply-To: <20090128020220.GE1321@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> PJ Hyett <pjhyett@gmail.com> wrote:
>>
>> Is there any possibility to have the server code in an upcoming
>> release account for clients running 1.6.1?
>
> I can't think off-hand of a way for the server to know what version
> the client is. There's nothing really different in the protocol
> between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
> include-tag) or later client.
Hmm, I am puzzled.
I do not know how 41fa7d2 (Teach git-fetch to exploit server side
automatic tag following, 2008-03-03), which is about the conversation
between fetch-pack and upload-pack, is relevant to the issue at hand,
which is about the conversation between send-pack and receive-pack.
In send-pack receive-pack protocol, the server talks first before
listening to the client, and the .have data is in this first part of the
conversation.
By the way, I think Documentation/technical/pack-protocol.txt needs to be
updated. send-pack receive-pack protocol uses C and S to mean receiver
and sender respectively. We should at least s/C/R/ that part, and
possibly add description about ".have" thing.
^ permalink raw reply
* How to install and use a custom merge driver
From: Alec Clews @ 2009-01-28 3:10 UTC (permalink / raw)
To: git
Background:
I want to use git to track the delivery of patch files into existing
file trees. This means that new files will need to be copied over
existing files (especially in the case of binary files or textual conflicts)
To this end I want to use a custom merge driver (actually the cp command)
Setup:
I have set up my ..git/info/gitattributes as follows
* merge=overwrite
I have defined in .git/config
[merge "overwrite"]
name = overwrite using cp
driver = cp %B %A
Problem:
However when I perform a git merge the default merge is being called. Is
there something else needed to make git perform the copy operation?
Many thanks
--
Alec Clews
Personal <alec.clews@gmail.com> Melbourne, Australia.
Jabber: alecclews@jabber.org.au PGPKey ID: 0x9BBBFC7C
Blog http://alecthegeek.wordpress.com/
^ permalink raw reply
* [PATCH v2 1/2] git-am: emit usage when called w/o arguments and w/o patch on stdin
From: Jay Soffian @ 2009-01-28 3:27 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, gitster, sverre
When git am is called w/o arguments, w/o a patch on stdin and the user hits
ctrl-c, it leaves behind a partially populated $dotest directory. After this
commit, it emits usage when called w/o arguments and w/o a patch on stdin.
Also ensure that $dotest is cleaned up if user manages to interupt mailsplit
while it is processing input, but not if mailsplit exits non-zero due to
error.
Noticed by Sverre Rabbelier
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Change from v1: don't remove $dotest if mailsplit exits non-zero
git-am.sh | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index b1c05c9..65285a0 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -254,10 +254,13 @@ else
done
shift
fi
- git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" || {
- rm -fr "$dotest"
+ test $# = 0 && test -t 0 && usage
+ trap 'rm -fr "$dotest"' 0
+ git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" || {
+ trap - 0
exit 1
}
+ trap - 0
# -s, -u, -k, --whitespace, -3, -C and -p flags are kept
# for the resuming session after a patch failure.
--
1.6.1.224.gb56c
^ permalink raw reply related
* [PATCH v2 2/2] git-am: minor cleanups
From: Jay Soffian @ 2009-01-28 3:27 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, gitster, sverre
In-Reply-To: <1233113262-17491-1-git-send-email-jaysoffian@gmail.com>
Update usage statement to remove a no-longer supported option, and to hide two
options (one a no-op, one internal) unless --help-all is used.
Use "test -t 0" instead of deprecated "tty -s" to detect when stdin is a
terminal. (test -t 0 is used elsewhere in git-am and in other git shell
scripts, tty -s is not.)
Use "test ..." instead of "[ ... ]" and "die <msg>" instead of "echo <msg>
>&2; exit 1" to be consistent with rest of script.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Added SoB, otherwise no change from v1.
git-am.sh | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index 65285a0..9653a98 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -8,9 +8,8 @@ OPTIONS_SPEC="\
git am [options] [<mbox>|<Maildir>...]
git am [options] (--resolved | --skip | --abort)
--
-d,dotest= (removed -- do not use)
i,interactive run interactively
-b,binary (historical option -- no-op)
+b,binary* (historical option -- no-op)
3,3way allow fall back on 3way merging if needed
s,signoff add a Signed-off-by line to the commit message
u,utf8 recode into utf8 (default)
@@ -24,7 +23,7 @@ resolvemsg= override error message when patch failure occurs
r,resolved to be used after a patch failure
skip skip the current patch
abort restore the original branch and abort the patching operation.
-rebasing (internal use for git-rebase)"
+rebasing* (internal use for git-rebase)"
. git-sh-setup
prefix=$(git rev-parse --show-prefix)
@@ -204,7 +203,7 @@ then
# unreliable -- stdin could be /dev/null for example
# and the caller did not intend to feed us a patch but
# wanted to continue unattended.
- tty -s
+ test -t 0
;;
*)
false
@@ -283,10 +282,7 @@ fi
case "$resolved" in
'')
files=$(git diff-index --cached --name-only HEAD --) || exit
- if [ "$files" ]; then
- echo "Dirty index: cannot apply patches (dirty: $files)" >&2
- exit 1
- fi
+ test "$files" && die "Dirty index: cannot apply patches (dirty: $files)"
esac
if test "$(cat "$dotest/utf8")" = t
--
1.6.1.224.gb56c
^ permalink raw reply related
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Shawn O. Pearce @ 2009-01-28 3:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: PJ Hyett, Johannes Schindelin, git
In-Reply-To: <7v3af4yvmu.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > PJ Hyett <pjhyett@gmail.com> wrote:
> >>
> >> Is there any possibility to have the server code in an upcoming
> >> release account for clients running 1.6.1?
> >
> > I can't think off-hand of a way for the server to know what version
> > the client is. There's nothing really different in the protocol
> > between a 1.6.1 client and a v1.5.5-rc0~44^2 (introduction of
> > include-tag) or later client.
>
> Hmm, I am puzzled.
>
> I do not know how 41fa7d2 (Teach git-fetch to exploit server side
> automatic tag following, 2008-03-03), which is about the conversation
> between fetch-pack and upload-pack, is relevant to the issue at hand,
> which is about the conversation between send-pack and receive-pack.
Oh, right, its not. I was pointing out that the last time the
protocol changed in a way the server can infer something about the
client, which IIRC was 41fa7d2, we still don't have a way to tell
what the client is.
> In send-pack receive-pack protocol, the server talks first before
> listening to the client, and the .have data is in this first part of the
> conversation.
But as you rightly point out, that's the real problem. Since the
server talks first, there's no way for the server to avoid giving
out the newer ".have" lines to a buggy client, as it knows nothing
at all about the client. Not even its capabilities.
PJ - the short story here is, to forever work around these buggy
1.6.1 clients, you'd have to either run an old server forever,
or forever run a patched server that disables the newer ".have"
extension in the advertised data written by git-upload-pack.
There just isn't a way to hide this from the client.
Really though, I'd recommend getting your users to upgrade to a
non-buggy client. Pasky has the same problem on repo.or.cz; if
he doesn't have it already he will soon when he upgrades...
--
Shawn.
^ permalink raw reply
* Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote
From: Junio C Hamano @ 2009-01-28 3:33 UTC (permalink / raw)
To: Björn Steinbrink
Cc: PJ Hyett, Shawn O. Pearce, Johannes Schindelin, Linus Torvalds,
git
In-Reply-To: <20090128013840.GA7224@atjola.homenet>
Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> Since 40c155ff14c, receive-pack on the remote also sends refs from its
> alternates. Unfortunately, we don't filter commits that don't exist in the
> local repository from that list. This made us pass those unknown commits
> to pack-objects, causing it to fail with a "bad object" error.
>
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
> builtin-send-pack.c | 14 +++++++++-----
> 1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index a9fdbf9..10d7016 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -52,11 +52,15 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
> * parameters by writing to the pipe.
> */
> for (i = 0; i < extra->nr; i++) {
> - memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
> - buf[0] = '^';
> - buf[41] = '\n';
> - if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
> - break;
> + if (!is_null_sha1(&extra->array[i][0]) &&
> + has_sha1_file(&extra->array[i][0])) {
> + memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
> + buf[0] = '^';
> + buf[41] = '\n';
> + if (!write_or_whine(po.in, buf, 42,
> + "send-pack: send refs"))
> + break;
> + }
> }
>
> while (refs) {
Actually I changed my mind.
We have the exactly the same issue for the real refs the target repository
has, not just borrowed phantom refs, in the code from day one of git-push.
In other words, this issue predates the ".have" extension, and your update
is in line with how the codepath for the real refs does its thing. So
your fix is not worse than the existing code.
It can be argued that at least in the "real ref" case you are in control
of both ends and if you have a disconnected chain in your local repository
that you do not have a ref for, you are screwing yourself, and it is your
problem. But when you forked your repository from somebody else on a
hosting site like github, you do not have much control over the other end
(because it is a closed site you cannot ssh in to diagnose what is really
going on), and if you do not exactly know from whom your hosted repository
is borrowing, it is more likely that you will get into a situation where
you may have objects near the tip without having the full chain after an
aborted transfer, and the insufficient check of doing only has_sha1_file()
may become a larger issue in such a settings.
But still, let's take the approach I labeled as *wrong* as an interim
solution for the immediate future.
I'd prefer a small helper function to consolidate the duplicated code,
like the attached patch, though. How about doing it like this?
builtin-send-pack.c | 46 ++++++++++++++++++++++++----------------------
1 files changed, 24 insertions(+), 22 deletions(-)
diff --git c/builtin-send-pack.c w/builtin-send-pack.c
index a9fdbf9..2d24cf2 100644
--- c/builtin-send-pack.c
+++ w/builtin-send-pack.c
@@ -15,6 +15,23 @@ static struct send_pack_args args = {
/* .receivepack = */ "git-receive-pack",
};
+static int feed_object(const unsigned char *theirs, int fd, int negative)
+{
+ char buf[42];
+
+ if (!has_sha1_file(theirs))
+ return 1;
+ /*
+ * NEEDSWORK: we should not be satisfied by simply having
+ * theirs, but should be making sure it is reachable from
+ * some of our refs.
+ */
+ memcpy(buf + negative, sha1_to_hex(theirs), 40);
+ if (negative)
+ buf[0] = '^';
+ buf[40 + negative] = '\n';
+ return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+}
/*
* Make a pack stream and spit it out into file descriptor fd
*/
@@ -35,7 +52,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
};
struct child_process po;
int i;
- char buf[42];
if (args.use_thin_pack)
argv[4] = "--thin";
@@ -51,31 +67,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
* We feed the pack-objects we just spawned with revision
* parameters by writing to the pipe.
*/
- for (i = 0; i < extra->nr; i++) {
- memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
- buf[0] = '^';
- buf[41] = '\n';
- if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+ for (i = 0; i < extra->nr; i++)
+ if (!feed_object(extra->array[i], po.in, 1))
break;
- }
while (refs) {
if (!is_null_sha1(refs->old_sha1) &&
- has_sha1_file(refs->old_sha1)) {
- memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
- buf[0] = '^';
- buf[41] = '\n';
- if (!write_or_whine(po.in, buf, 42,
- "send-pack: send refs"))
- break;
- }
- if (!is_null_sha1(refs->new_sha1)) {
- memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
- buf[40] = '\n';
- if (!write_or_whine(po.in, buf, 41,
- "send-pack: send refs"))
- break;
- }
+ !feed_object(refs->old_sha1, po.in, 1))
+ break;
+ if (!is_null_sha1(refs->new_sha1) &&
+ !feed_object(refs->new_sha1, po.in, 0))
+ break;
refs = refs->next;
}
.
^ permalink raw reply related
* Re: Heads up: rebase -i -p will be made sane again
From: Stephen Haberman @ 2009-01-28 3:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901280225240.3586@pacific.mpi-cbg.de>
> Actually, I misread t3410 a great deal. The situation is as follows:
>
> ... UPSTREAM
> \
> ... A - B - C -D
>
> A is a patch the upstream does not have, B is a patch UPSTREAM has,
> and "git diff C^!" (i.e. the diff of C to its first parent) is _also_
> identical to a diff of a merge that is in UPSTREAM.
>
> Basically, t3410 tests that after "git rebase -i -p UPSTREAM" and leaving
> the rebase script as-is, essentially, A and D are cherry-picked on top of
> UPSTREAM.
Cool--I "knew" that, but could not have articulated the case as
succinctly.
> > Does this mean you're just getting rid of the code that calls "rev list
> > --cherry-pick"?
>
> Only now do I understand.
>
> I misread the code for --cherry-pick. For merges, it adds the diff to the
> first parent!
Ah, so that is how --cherry-pick works--I'd never looked into the
patch-id stuff before. Makes sense, both of how it is leveraged by
rev-list --cherry-pick and also that it doesn't make sense to only be
against the first parent of merges.
> So I adapted my code to find the "dropped" merges in
> git-rebase--interactive, too, for now, but I guess the proper fix is
> something like this:
So, if C, as a merge commit, doesn't get a patch id anymore (right?),
does that mean that C is included with A and D in the cherry-picking
on top of UPSTREAM (because with no patch id it cannot be recognized
as a duplicate)? So then C' is an empty-commit? This would be fine, I
think, or can you detect that C is a noop somehow without patch ids?
Thanks,
Stephen
^ permalink raw reply
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Stephen Bannasch @ 2009-01-28 3:52 UTC (permalink / raw)
To: git
In-Reply-To: <20090128033020.GF1321@spearce.org>
At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:
>PJ - the short story here is, to forever work around these buggy
>1.6.1 clients, you'd have to either run an old server forever,
>or forever run a patched server that disables the newer ".have"
>extension in the advertised data written by git-upload-pack.
>There just isn't a way to hide this from the client.
>
>Really though, I'd recommend getting your users to upgrade to a
>non-buggy client. Pasky has the same problem on repo.or.cz; if
>he doesn't have it already he will soon when he upgrades...
Do you know if this problem is fixed in tag v1.6.1.1?
Tagger: Junio C Hamano <gitster@pobox.com>
Date: Sun Jan 25 12:41:48 2009 -0800
commit 5c415311f743ccb11a50f350ff1c385778f049d6
^ permalink raw reply
* Re: Bad objects error since upgrading GitHub servers to 1.6.1
From: Shawn O. Pearce @ 2009-01-28 3:57 UTC (permalink / raw)
To: Stephen Bannasch; +Cc: git
In-Reply-To: <p06240812c5a58676a1e2@[63.138.152.192]>
Stephen Bannasch <stephen.bannasch@deanbrook.org> wrote:
> At 7:30 PM -0800 1/27/09, Shawn O. Pearce wrote:
>> PJ - the short story here is, to forever work around these buggy
>> 1.6.1 clients, you'd have to either run an old server forever,
>> or forever run a patched server that disables the newer ".have"
>> extension in the advertised data written by git-upload-pack.
>> There just isn't a way to hide this from the client.
>>
>> Really though, I'd recommend getting your users to upgrade to a
>> non-buggy client. Pasky has the same problem on repo.or.cz; if
>> he doesn't have it already he will soon when he upgrades...
>
> Do you know if this problem is fixed in tag v1.6.1.1?
>
> Tagger: Junio C Hamano <gitster@pobox.com>
> Date: Sun Jan 25 12:41:48 2009 -0800
> commit 5c415311f743ccb11a50f350ff1c385778f049d6
Without even checking Git I can tell you it isn't fixed by 1.6.1.1.
The date on the tag is Jan 25th, 2 full days before PJ reported
the problem and a solution was proposed...
--
Shawn.
^ permalink raw reply
* Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote
From: Björn Steinbrink @ 2009-01-28 3:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: PJ Hyett, Shawn O. Pearce, Johannes Schindelin, Linus Torvalds,
git
In-Reply-To: <7vskn4xfyg.fsf@gitster.siamese.dyndns.org>
On 2009.01.27 19:33:11 -0800, Junio C Hamano wrote:
> It can be argued that at least in the "real ref" case you are in control
> of both ends and if you have a disconnected chain in your local repository
> that you do not have a ref for, you are screwing yourself, and it is your
> problem. But when you forked your repository from somebody else on a
> hosting site like github, you do not have much control over the other end
> (because it is a closed site you cannot ssh in to diagnose what is really
> going on), and if you do not exactly know from whom your hosted repository
> is borrowing, it is more likely that you will get into a situation where
> you may have objects near the tip without having the full chain after an
> aborted transfer, and the insufficient check of doing only has_sha1_file()
> may become a larger issue in such a settings.
Uhm, it might be obvious, but what exactly could go wrong? Do we need to
fetch from multiple repos when alternates are involved? Or how would we
end up with a broken chain? I mean, it starts to make some sense to me
why we would need the connectivity check, but how do we end up with a
"partial" fetch at all?
> I'd prefer a small helper function to consolidate the duplicated code,
> like the attached patch, though. How about doing it like this?
Yeah, that looks a lot nicer :-)
Björn
^ permalink raw reply
* Re: Heads up: rebase -i -p will be made sane again
From: Johannes Schindelin @ 2009-01-28 4:01 UTC (permalink / raw)
To: Stephen Haberman; +Cc: git, gitster
In-Reply-To: <20090127213950.3596ecf9.stephen@exigencecorp.com>
Hi,
On Tue, 27 Jan 2009, Stephen Haberman wrote:
> > So I adapted my code to find the "dropped" merges in
> > git-rebase--interactive, too, for now, but I guess the proper fix is
> > something like this:
>
> So, if C, as a merge commit, doesn't get a patch id anymore (right?),
> does that mean that C is included with A and D in the cherry-picking
> on top of UPSTREAM (because with no patch id it cannot be recognized
> as a duplicate)?
Yep, it gets into the list. But not with a "pick" command, as a merge it
will get a "merge" command.
> So then C' is an empty-commit? This would be fine, I think, or can you
> detect that C is a noop somehow without patch ids?
Actually, there are three possible outcomes:
- it tries to merge an ancestor of HEAD or HEAD itself -> noop
- it tries to merge which results in a fast-forward -> fine
- it tries to merge and a proper merge is necessary -> may conflict
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote
From: Junio C Hamano @ 2009-01-28 4:13 UTC (permalink / raw)
To: Björn Steinbrink
Cc: PJ Hyett, Shawn O. Pearce, Johannes Schindelin, Linus Torvalds,
git
In-Reply-To: <20090128035804.GC7503@atjola.homenet>
Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> Uhm, it might be obvious, but what exactly could go wrong?
Between the refs and your object store, there is a contract that
guarantees that everything that is reachable from your refs are complete
and you won't hit unreachable object while traversing the reachability
chain from them. But your object store can contain other garbage
objects. The contract is one of the things "git fsck" checks.
Imagine you have fetched from somewhere with a commit walker (e.g. fetch
over http), that started fetching from the tip commit and its associated
objects, and then got interrupted. Such a transfer will leave the objects
in your local repository but it is safe because it won't update your refs.
>> I'd prefer a small helper function to consolidate the duplicated code,
>> like the attached patch, though. How about doing it like this?
>
> Yeah, that looks a lot nicer :-)
But it was broken. The initial check feed_object() does with
has_sha1_file() and NEEDSWORK comment needs to be inside
if (negative) {
if (!has_sha1_file(theirs))
return 1;
/*
* NEEDSWORK: we should not be satisfied by simply having
* theirs, but should be making sure it is reachable from
* some of our refs.
*/
}
to make sure we won't trigger the availability or connectivity check for
positive refs.
^ 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