Fwd: git-remote-fd problem
The remote-fd expects the transport to pass half-closes. So you can't
close all at once.
Let there be pipes W and R and transport connection C.
- W-read should be closed after being passed to remote-fd.
- R-write should be closed after being passed to remote-fd.
- Upon receiving no more data from C, close W-write.
- Upon receiving EOF from R-read, close it and signal no more data
to C.
Hi, I followed your advices, correctly close pipes but git clone still
doesnt finish and hanging on.
Code is in an attachement (its part of big system).
#create pipes
w_pipe = os.pipe()
r_pipe = os.pipe()
client_process = subprocess.Popen(/usr/bin/git clone fd::{0},{1} /tmp/gittest.format(r_pipe[0], w_pipe[1]), shell=True)
#closing pipes
os.close(r_pipe[0])
os.close(w_pipe[1])
epoll = select.epoll()
epoll.register(w_pipe[0], select.EPOLLIN)
epoll.register(proc.fd, select.EPOLLIN)
remoteGit = proc.runDaemon(git-upload-pack /tmp/testgit)
while True:
events = epoll.poll(1)
for fd, event in events:
if fd == w_pipe[0]:
if event select.EPOLLIN:
rd = os.read(w_pipe[0], 1)
if rd:
#write data to remove git server
remoteGit.writeToChannel(rd)
else:
proc.writeError(Local socket write error)
return 1
else:
proc.writeError(Local socket error)
return 1
elif fd == proc.fd:
if event select.EPOLLIN:
#read data from remote git server
data = remoteGit.getAll()
remoteGit.stderrWrite()
if not data:
#remote server send EOF, close local pipe
#but git clone is still running
os.close(r_pipe[1])
return 0
want = len(data)
writed = 0
offset = 0
while(writed != want):
#write data from remote git server to local pipe
wr = os.write(r_pipe[1], data[offset:])
if(wr 0):
return 1
writed += wr
offset += wr
else:
return -1
Re: Git's Perl scripts can fail if user is configured for perlbrew
On Sun, Dec 28, 2014 at 11:36 PM, Randy J. Ray rj...@blackperl.com wrote: I use git on MacOS via homebrew (http://brew.sh/), and a custom Perl installation built and managed via perlbrew (http://perlbrew.pl/). At some point, commands like git add -i broke. I say at some point, because I'm not a git power-user and I only just noticed it this week. I am running Git 2.2.1 with a perlbrew'd Perl 5.20.1. When I would run git add -i (or git add -p), it would immediately die with a signal 11. Some poking around showed that those git commands that are implemented as Perl scripts run under /usr/bin/perl, and also prefix some directories to the module search-path. The problem stems from the fact that, when you are using perlbrew, you also have the PERL5LIB environment variable set. The contents of it lay between the git-provided paths and the default contents of @INC. When the Git module is loaded, it (eventually) triggers a load of List::Util, whose C-level code fails to load because of a version mismatch; you got List::Util from the paths in PERL5LIB, but it doesn't match the version of perl from /usr/bin/perl. After poking around and trying a few different things, I have found that using the following line in place of #!/usr/bin/perl solves this problem: #!/usr/bin/env perl This can be done by defaulting PERL_PATH to /usr/bin/env perl in Makefile. I don't know enough about the overall git ecosystem to know if this would have an adverse effect on anything else (in particular, Windows compatibility, but then Windows probably isn't having this issue in the first place). I could just create and mail in the one-line patch for this, but I thought it might be better to open it up for some discussion first? [CC'd the perlbrew author] This is a bit of a tricky issue. Using whatever perl is defined in the environment is just as likely to break, in general the build process tries to pick these assets at compile-time. Imagine you're experimenting with some custom perl version and now Git inexplicably breaks. It's better if Git detects a working perl when you compile it and sticks with that, which is why we use /usr/bin/perl by default. When you're setting PERL5LIB you're indicating to whatever perl interpreter you're going to run that that's where they it should pick up its modules. IMO they way perlbrew does this is broken, instead of setting PATH + PERL5LIB globally for your login shell it should set the PATH, and then the perl in that path should be a pointer to some small shellscript that sets PERL5LIB for *its* perl. I don't know what the right tradeoff here is, but I think it would be just as sensible to unset PERL5LIB in our own perl scripts + modules, it would make live monkeypatching when you wanted to harder, but we could always add a GITPERL5LIB or something... -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's Perl scripts can fail if user is configured for perlbrew
On 2014-12-28 23.36, Randy J. Ray wrote: I use git on MacOS via homebrew (http://brew.sh/), and a custom Perl installation built and managed via perlbrew (http://perlbrew.pl/). At some point, commands like git add -i broke. I say at some point, because I'm not a git power-user and I only just noticed it this week. I am running Git 2.2.1 with a perlbrew'd Perl 5.20.1. When I would run git add -i (or git add -p), it would immediately die with a signal 11. Some poking around showed that those git commands that are implemented as Perl scripts run under /usr/bin/perl, and also prefix some directories to the module search-path. The problem stems from the fact that, when you are using perlbrew, you also have the PERL5LIB environment variable set. The contents of it lay between the git-provided paths and the default contents of @INC. When the Git module is loaded, it (eventually) triggers a load of List::Util, whose C-level code fails to load because of a version mismatch; you got List::Util from the paths in PERL5LIB, but it doesn't match the version of perl from /usr/bin/perl. After poking around and trying a few different things, I have found that using the following line in place of #!/usr/bin/perl solves this problem: #!/usr/bin/env perl This can be done by defaulting PERL_PATH to /usr/bin/env perl in Makefile. I don't know enough about the overall git ecosystem to know if this would have an adverse effect on anything else (in particular, Windows compatibility, but then Windows probably isn't having this issue in the first place). I could just create and mail in the one-line patch for this, but I thought it might be better to open it up for some discussion first? Randy Having problems with different perl installations is not an unknown problem in Git, I would say. And Git itself is prepared to handle this situation: In Makefile I can read: # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl). (What Git can not decide is which perl it should use, the one pointed out by $PATH or /usr/bin/perl.) What does type perl say ? And what happens when you build and install Git like this: PERL_PATH=/XX/YY/perl make install --- Are you thinking about changing ifndef PERL_PATH PERL_PATH = /usr/bin/perl endif -- into -- ifndef PERL_PATH PERL_PATH = $(shell which perl) endif --- At first glance that could make sense, at least to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t4255: test am submodule with diff.submodule
Eric Sunshine sunsh...@sunshineco.com writes: + (git am --abort || true) Why (x || y)? Is 'x' so unreliable that we do not know how should exit? Should this be test_must_fail git am --abort? + (cd submodule git rev-parse HEAD ../actual) git -C submodule rev-parse HEAD actual perhaps? +test_expect_success 'diff.submodule unset' ' + (git config --unset diff.submodule || true) I think test_config and test_unconfig were invented for things like this (same for all the other use of git config). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/1] http: Add Accept-Language header if possible
Eric Sunshine sunsh...@sunshineco.com writes: Just a few comments and observations below. Alone, they are not necessarily worth a re-roll, but if you happen to re-roll for some other reason, perhaps take them into consideration. I actually think everything you said in this review makes sense and will make the resulting code a lot better (especially the part on the parsing loop). Thanks, as usual, for a careful reading. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GIT_PUSH_CERT* env vars and update/post-update hooks...
Sitaram Chamarty sitar...@gmail.com writes: Any chance I could persuade you to set the GIT_PUSH_CERT* environment variables for the update (and post-update) hooks also? I do not think of a fundamental reason why we shouldn't give these environment variables to update or other hooks. It should just be the matter of calling prepare_push_cert_sha1() on the child_process struct used to run the hooks you want. [1]: because it's nice to *selectively* reject refs when more than one ref is pushed at the same time; pre-receive is all or none. It cuts both ways; inside update, your selective rejection cannot make the decision with the whole picture (you only have a peephole into individual changes). post-receive sees the whole picture, but it has to say all-or-none. Neither is ideal if you truly want a useful selective. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Documentation/SubmittingPatches: Explain the rationale of git notes
On Mon, Dec 22, 2014 at 9:55 AM, Junio C Hamano gits...@pobox.com wrote: Stefan Beller sbel...@google.com writes: This adds an explanation of why you want to have the --notes option given to git format-patch. Signed-off-by: Stefan Beller sbel...@google.com --- Notes: with optionally update Documentation/SubmittingPatches to point at the file. That file actually talks about notes already. I am sending two patches touching that file, one of them explaining the --notes workflow rationale and one of them just changing white spaces. A few weeks ago I wanted to patch format-patch to remove change ids. This is not needed any more for the git workflow as I disabled them and do not upload any patches to an optional Gerrit code review server anymore. I do like the workflow using --notes as well from a developers perspective as I take literally notes for my own sanity. I wonder if I should add a config format.notes = [default-off, always, on-if-non-empty] so I don't need always add --notes manually to the command line. Thanks, Stefan Documentation/SubmittingPatches | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index fa71b5f..16b5d65 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -176,7 +176,11 @@ message starts, you can put a From: line to name that person. You often want to add additional explanation about the patch, other than the commit message itself. Place such cover letter material between the three dash lines and the diffstat. Git-notes -can also be inserted using the `--notes` option. +can also be inserted using the `--notes` option. If you are one Because the previous sentence is talking about the cover letter to describe the overall series, it probably is a good idea to add after the three-dashes of each patch after ... using the notes option for clarity, perhaps? The previous sentence is talking about additional information, which should not go into the commit message. (very similar to the cover letter but just for one patch). It already mentions the place between the three dash lines and the diffstat. +of those developers who cannot write perfect code the first time +and need multiple iterations of review and discussion, you are +encouraged to use the notes to describe the changes between the +different versions of a patch. Perhaps s/you are encouraged to/you may want to/? It might be better to be even more explicit, e.g. you may want to keep track of the changes between the versions using the notes and then use `--notes` when preparing the series for submission? I did reword it to make it more obvious. Do not attach the patch as a MIME attachment, compressed or not. Do not let your e-mail client send quoted-printable. Do not let Thanks. A paragraph before that change we have git format-patch command follows the best current practice to format the body of an e-mail message. which makes me wonder if the notes are a best practice or may be becoming a best practice. So maybe we want to default to add the notes in format-patch instead of explicitly asking for it. But that's just an idea suited for another patch. Thanks -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs: release strbuf after use in check_refname_component()
René Scharfe l@web.de writes: Signed-off-by: Rene Scharfe l@web.de --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 5fcacc6..ed3b2cb 100644 --- a/refs.c +++ b/refs.c @@ -2334,7 +2334,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, struct strbuf err = STRBUF_INIT; unable_to_lock_message(ref_file, errno, err); error(%s, err.buf); - strbuf_reset(err); + strbuf_release(err); goto error_return; } } The subject does not seem to match what the patch is doing, but the patch is obviously correct ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] Documentation/SubmittingPatches: Explain the rationale of git notes
This adds more explanation of why you want to have the --notes option given to git format-patch. Signed-off-by: Stefan Beller sbel...@google.com --- Notes: Changes v2: * s/you are encouraged to/you may want to/ * a stronger hint to use the git notes and then --notes for format-patch. Documentation/SubmittingPatches | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e3c942e..f42c607 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -177,7 +177,12 @@ message starts, you can put a From: line to name that person. You often want to add additional explanation about the patch, other than the commit message itself. Place such cover letter material between the three dash lines and the diffstat. Git-notes -can also be inserted using the `--notes` option. +can also be inserted using the `--notes` option. If you are one +of those developers who cannot write perfect code the first time +and need multiple iterations of review and discussion, you may +want to keep track of the changes between different versions of +a patch using notes and then also use the `--notes` option when +preparing the patch for submission. Do not attach the patch as a MIME attachment, compressed or not. Do not let your e-mail client send quoted-printable. Do not let -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] send-email: Improve format of smtp initialization error message
Signed-off-by: Alexander Kuleshov kuleshovm...@gmail.com
---
git-send-email.perl | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 82c6fea..60dcd8d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1275,10 +1275,10 @@ X-Mailer: git-send-email $gitversion
if (!$smtp) {
die Unable to initialize SMTP properly. Check config
and use --smtp-debug. ,
- VALUES: server=$smtp_server ,
- encryption=$smtp_encryption ,
- hello=$smtp_domain,
- defined $smtp_server_port ?
port=$smtp_server_port : ;
+ \nVALUES: \n\tserver=$smtp_server ,
+ \n\tencryption=$smtp_encryption ,
+ \n\thello=$smtp_domain,
+ defined $smtp_server_port ?
\n\tport=$smtp_server_port : ;
}
smtp_auth_maybe or die $smtp-message;
--
2.2.1.201.gbbcefff.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-log: added --invert-grep option
Christoph Junghans ott...@gentoo.org writes: Ok, I drafted a first version of the suggest --grep-begin ... --grep-end syntax. I am somewhat surprised that it was doable that cleanly. The syntax, as I already said, is a bit too ugly to live in that form I suggested, though ;-). However, I could not find a good ways to invert the match on a commit basis instead of the normal line-wise version. Good point. The only interface to tweak the way the individual matches are turned into file-level match (by default, we say any one of the clauses match, we find the file to be interesting) is --all-match (all of the clauses have to trigger at least once for us to find the file interesting), which, compared to the richer set of boolean operations at the line level, is a kludge. Offhand, short of overhauling that kludge to allow us to express The file has to have either 'atomic' or 'all-or-none' appear somewhere, and also cannot have 'wip' anywhere, I do not think of a good way, other than adding yet another kludge on top of the --all-match that says none of the clauses must trigger to express what you want to see happen in git log to exclude ones that are marked with wip, perhaps naming it --none-match or something. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] push.c: add an --atomic argument
On Thu, Dec 25, 2014 at 11:17 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 12/19/2014 08:39 PM, Stefan Beller wrote: Add a command line argument to the git push command to request atomic pushes. [...] diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 21b3f29..da63bdf 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects SYNOPSIS [verse] -'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=git-receive-pack] +'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=git-receive-pack] [--repo=repository] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream] [--signed] [--force-with-lease[=refname[:expect]]] @@ -136,6 +136,11 @@ already exists on the remote side. logged. See linkgit:git-receive-pack[1] for the details on the receiving end. +--atomic:: + Use an atomic transaction on the remote side if available. + Either all refs are updated, or on error, no refs are updated. + If the server does not support atomic pushes the push will fail. + [...] I'd like to discuss the big picture around this feature. I don't think that any of these questions are blockers, with the possible exception of the question of whether --atomic should fall back to non-atomic if the server doesn't support atomic pushes. 1. Should --atomic someday become the default? You seem to imply that --atomic might become the default sometime in the future. (I realize that this patch series does not propose to change the default but let's talk about it anyway.) In the real world, the most common reason for an --atomic push to fail would be that somebody else has pushed to a branch since our last update, resulting in a non-ff error. Would I find out about such an error before or after I have transferred my objects to the server? Another idea which I picked up from later patches in Ronnies original series would be to have a hidden refs directory at the server side. Then the push command would first transfer all the objects to this hidden refs space and at the very end the server would just update all the refs atomically. If that fails because somebody else has just pushed stuff to one of the branches you intended to update, the changes are still at the server in the hidden space. So if you just did a pull/merge on that branch and then try to push again, you would not have to transmit the objects again as they are already there at the server. These changes in such a hidden space on the server side could be just normal refs just not(never!) advertised to users. If I only find out at the end of the transfer, then it could be a pretty frustrating experience pushing a lot of references to a server over a slow connection. After waiting for a long transfer to complete the user would find out that the push was rejected and everything has to be done again from scratch. In such cases non---atomic behavior might be attractive: any references that can be updated should be updated, so that not *all* of the objects have to be pushed again. Even *if* --atomic becomes the default, we would certainly want to support a --no-atomic (or --non-atomic?) option to get the old behavior. It might be a good idea to add that option now, so that forward-looking script writers can start explicitly choosing --atomic vs. --no-atomic. That's a good point. Though I am not sure where you'd want me to add the --no-atomic flag. I don't think we'll change send-pack as it's plumbing. So current unmaintained scripts should continue to work the way they always did, i.e. having the --no-atomic behavior. The atomic behavior made default would just be part of git push, which is porcelain which is expected to change? But there it makes sense to have a --no-atomic option in place. 2. Is this an option that users will want to specify via the command line? For scripts that want to insist on atomic updates, it is no problem to specify --atomic on the command line. But supposing that --atomic is a good default for some people, it would be awkward for them to have to specify it on every git push invocation. It therefore might be nice to have a configuration setting to choose whether --atomic is the default. That's true. Would it make sense to include that in this series or delay it for another series? I don't want to make the series so large again so it becomes reviewer-unfriendly. Also (see above) it might be useful to set --atomic only for particular servers (for example, only for those to which you have a fast connection). This suggests that the atomic/non-atomic configuration should be settable on a per-remote basis. So on the client side you want to configure on a per-remote basis which default behavior to use and also at the
Re: [PULL] git svn updates for master
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] send-email: Improve format of smtp initialization error message
Alexander Kuleshov kuleshovm...@gmail.com writes:
Signed-off-by: Alexander Kuleshov kuleshovm...@gmail.com
---
git-send-email.perl | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 82c6fea..60dcd8d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1275,10 +1275,10 @@ X-Mailer: git-send-email $gitversion
if (!$smtp) {
die Unable to initialize SMTP properly. Check config
and use --smtp-debug. ,
- VALUES: server=$smtp_server ,
- encryption=$smtp_encryption ,
- hello=$smtp_domain,
- defined $smtp_server_port ?
port=$smtp_server_port : ;
+ \nVALUES: \n\tserver=$smtp_server ,
+ \n\tencryption=$smtp_encryption ,
+ \n\thello=$smtp_domain,
+ defined $smtp_server_port ?
\n\tport=$smtp_server_port : ;
It may be a good convention to have LF at the beginning of a new
string (i.e. we terminate the old line only when we have something
more to say), but that is true only when we want to end the sentence
without the final newline. I wonder if that is true in this case;
do we want perl to say at line # in file X at the end?
In any case, you have two output lines that ends with a trailing SP
just before LF, which is probably not what you wanted.
If we want to see all lines end with LF, it may be far easier to
read this way:
die msg\n,
\tvar1=val1\n,
\tvar2=val2\n,
defined $var3 ? \tvar3=val3\n : ;
I dunno.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] checkout-index: fix --temp relative path mangling
Eric Sunshine sunsh...@sunshineco.com writes: Fix this by taking advantage of write_name_quoted_relative() to recover the original name properly, rather than assuming that it can be recovered by skipping strlen(prefix) bytes. Nice; I was wondering if we already had that helper when I saw the initial report of the problem. Thanks for a nicely constructed series. Will queue. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()
Christian Couder chrisc...@tuxfamily.org writes:
When running for example git bisect bad HEAD or
git bisect good master, the parameter passed to
git bisect (bad|good) has to be parsed into a
commit hash before checking if it is the expected
commit or not.
Hmm, is that because you wrote commit object name in 40-hex in the
EXPECTED_REV and you need to compare with what the user gave you
which could be symbolic?
The conversion makes sense, but why is it a bad thing to say
git bisect bad maint
when 'maint' is not what you checked out in the current bisect run
in the first place (perhaps you checked if it is good or bad manually
before you started bisecting)?
diff --git a/git-bisect.sh b/git-bisect.sh
index 6cda2b5..2fc07ac 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,15 +237,18 @@ bisect_state() {
check_expected_revs $rev ;;
2,bad|*,good|*,skip)
This part accepts arbitrary number of revs when running good and
skip, e.g.
git bisect good maint master next
and it loops
shift
- eval=''
+ hash_list=''
for rev in $@
...
+ for rev in $hash_list
+ do
+ bisect_write $state $rev
+ done
+ check_expected_revs $hash_list ;;
But check_expected_revs loops and leaves the loop early when it
finds anything that is not expected.
... goes and looks ...
Hmph, I think the logic in check_expected_revs is not wrong, but
this helper function is grossly misnamed. It is not checking and
rejecting the user input---it is checking to see if it can bypass
check_good_are_ancestors_of_bad() which is expensive, so when it
sees any one of the input is not what it checked out, it just
disables the optimization.
OK, will queue.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: git-remote-fd problem
On Mon, Dec 29, 2014 at 10:47:58AM +0100, Jiri Sevcik wrote:
The remote-fd expects the transport to pass half-closes. So you can't
close all at once.
Let there be pipes W and R and transport connection C.
- W-read should be closed after being passed to remote-fd.
- R-write should be closed after being passed to remote-fd.
- Upon receiving no more data from C, close W-write.
- Upon receiving EOF from R-read, close it and signal no more data
to C.
Hi, I followed your advices, correctly close pipes but git clone still
doesnt finish and hanging on.
Code is in an attachement (its part of big system).
Few ideas:
- Check that git clone (and its subprocesses) don't inherit
w_pipe[0] (/proc/pid/fd on Linux might be handy). If they do, that
prevents this program from closing the pipe.
- Setting environment variable GIT_TRANSLOOP_DEBUG to 1 might make
git spew lots of messages to stderr about reads, writes and closes.
#create pipes
w_pipe = os.pipe()
r_pipe = os.pipe()
client_process = subprocess.Popen(/usr/bin/git clone fd::{0},{1}
/tmp/gittest.format(r_pipe[0], w_pipe[1]), shell=True)
#closing pipes
os.close(r_pipe[0])
os.close(w_pipe[1])
epoll = select.epoll()
epoll.register(w_pipe[0], select.EPOLLIN)
epoll.register(proc.fd, select.EPOLLIN)
remoteGit = proc.runDaemon(git-upload-pack /tmp/testgit)
while True:
events = epoll.poll(1)
for fd, event in events:
if fd == w_pipe[0]:
if event select.EPOLLIN:
rd = os.read(w_pipe[0], 1)
if rd:
#write data to remove git server
remoteGit.writeToChannel(rd)
else:
proc.writeError(Local socket write error)
return 1
else:
proc.writeError(Local socket error)
return 1
elif fd == proc.fd:
if event select.EPOLLIN:
#read data from remote git server
data = remoteGit.getAll()
remoteGit.stderrWrite()
if not data:
#remote server send EOF, close local pipe
#but git clone is still running
os.close(r_pipe[1])
return 0
want = len(data)
writed = 0
offset = 0
while(writed != want):
#write data from remote git server to local pipe
wr = os.write(r_pipe[1], data[offset:])
if(wr 0):
return 1
writed += wr
offset += wr
else:
return -1
-Ilari
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] push.c: add an --atomic argument
Michael Haggerty mhag...@alum.mit.edu writes:
I'd like to discuss the big picture around this feature. I don't think
that any of these questions are blockers, with the possible exception of
the question of whether --atomic should fall back to non-atomic if the
server doesn't support atomic pushes.
1. Should --atomic someday become the default?
You seem to imply that --atomic might become the default sometime in
the future. (I realize that this patch series does not propose to change
the default but let's talk about it anyway.) In the real world, the most
common reason for an --atomic push to fail would be that somebody else
has pushed to a branch since our last update, resulting in a non-ff
error. Would I find out about such an error before or after I have
transferred my objects to the server?
That question is pretty much rhetorical, as certain rejections you
cannot fundamentally implement without having the data at hand.
If I only find out at the end of the transfer, then it could be a pretty
frustrating experience pushing a lot of references to a server over a
slow connection.
We'd like to have a long overdue protocol update for fetch and push
soonish anyawy (perhaps in the first half of 2015) and part of that
should include unified logic for common ancestor negotiation between
fetch and push [*1*]. We should be able to ease that with an
optimization similar to quickfetch done on the fetch side once that
happens.
Even *if* --atomic becomes the default, we would certainly want to
support a --no-atomic (or --non-atomic?) option to get the old
behavior. It might be a good idea to add that option now, so that
forward-looking script writers can start explicitly choosing --atomic
vs. --no-atomic.
Perhaps. But on the other hand, pushing multiple refs at the same
time is a sign that they are related and need to go together. The
reason why one but not others fails would be an indication that
there is somebody else pushing into the same repository and the
pusher and the other party are stepping on each other's toes, which
should be resolved primarily by inter-developer communication, not
with --no-atomic workaround.
2. Is this an option that users will want to specify via the command line?
For scripts that want to insist on atomic updates, it is no problem to
specify --atomic on the command line.
But supposing that --atomic is a good default for some people, it
would be awkward for them to have to specify it on every git push
invocation. It therefore might be nice to have a configuration setting
to choose whether --atomic is the default.
Also (see above) it might be useful to set --atomic only for
particular servers (for example, only for those to which you have a fast
connection). This suggests that the atomic/non-atomic configuration
should be settable on a per-remote basis.
I think you are hinting to have remote.atomicPush = {yes,no} that is
weaker than remote.$nick.atomicPush = {yes,no} or something like
that. I agree that would be a good direction to go.
3. What should happen if the server doesn't support atomic pushes?
If you asked for atomic push explicitly and if the server cannot
support it, push should fail.
If the only reason we are doing atomic is because in some future we
flipped the default (i.e. no remote.*.atomicPush or --atomic option
from the command line), then it might be OK to continue with a
warning.
I however think that the automatic demotion is too much complexity
for such a simple option. Please be atomic if you can but I'd take
non-atomic one if you do not want to give me atomic one that is
responded by I'd do non-atomic then, as you are perfectly happy
without is not very useful---such a pusher should just say I
accept non-atomic, which is what --no-atomic is for.
[Footnotes]
*1* Two other big ones are syntax change to have an explicit
extension packets instead of hiding the capability after NUL,
and resolving the who speaks first issue.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's Perl scripts can fail if user is configured for perlbrew
On 12/29/14, 7:21 AM, Ævar Arnfjörð Bjarmason wrote: [CC'd the perlbrew author] This is a bit of a tricky issue. Using whatever perl is defined in the environment is just as likely to break, in general the build process tries to pick these assets at compile-time. Imagine you're experimenting with some custom perl version and now Git inexplicably breaks. It's better if Git detects a working perl when you compile it and sticks with that, which is why we use /usr/bin/perl by default. These are good points. I'm not sure when this stopped working for me... I don't use the -i or -p options to git add very often. So I can't say at what point it stopped working with the current configuration, only that it used to work. When you're setting PERL5LIB you're indicating to whatever perl interpreter you're going to run that that's where they it should pick up its modules. IMO they way perlbrew does this is broken, instead of setting PATH + PERL5LIB globally for your login shell it should set the PATH, and then the perl in that path should be a pointer to some small shellscript that sets PERL5LIB for *its* perl. That would be for the perlbrew author to consider, of course. I don't know what the right tradeoff here is, but I think it would be just as sensible to unset PERL5LIB in our own perl scripts + modules, it would make live monkeypatching when you wanted to harder, but we could always add a GITPERL5LIB or something... You would have to have a shell script that un-set PERL5LIB and then invoked the given git script, because by the time script execution has begun, the contents of PERL5LIB have already been added to the head of the list of search paths. One approach I tried was to set the environment variable GITPERLLIB (which you already use and recognize, so there is no need for GITPERL5LIB), but that did not help. The base problem still remained: The content of PERL5LIB (which pointed to 5.20.1-compiled extensions) took priority over the default @INC contents (which were for a 5.16.2 perl). I don't know the right trade-off, either. I started out reporting this as an issue against the homebrew project's recipe for git, because they actually add more explicit library paths to @INC than a vanilla build/install of git does. But the problem is really in the interaction between /usr/bin/perl and a PERL5LIB set for an alternate perl. So the solution, if there is one, will lay here in git somewhere... Randy -- Randy J. Ray Sunnyvale, CA http://www.dereferenced.com rj...@blackperl.com twitter.com/rjray Silicon Valley Scale Modelers: http://www.svsm.org -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs: release strbuf after use in check_refname_component()
On Mon, Dec 29, 2014 at 09:37:43AM -0800, Junio C Hamano wrote: René Scharfe l@web.de writes: Signed-off-by: Rene Scharfe l@web.de --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 5fcacc6..ed3b2cb 100644 --- a/refs.c +++ b/refs.c @@ -2334,7 +2334,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, struct strbuf err = STRBUF_INIT; unable_to_lock_message(ref_file, errno, err); error(%s, err.buf); - strbuf_reset(err); + strbuf_release(err); goto error_return; } } The subject does not seem to match what the patch is doing, but the patch is obviously correct ;-) The worst part of this is that I got it right in my hacked-up version: http://article.gmane.org/gmane.comp.version-control.git/259853 but then after much discussion, we dropped all of the lead-in patches, and I sent Ronnie's unedited: http://article.gmane.org/gmane.comp.version-control.git/259911 All that looking and I didn't notice the release/reset difference between our two versions. Sheesh. Which is all a roundabout way of saying yes, René's patch is obviously correct. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Dec 2014, #05; Mon, 29)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. It has been understandably somewhat a slow week, and this will be the last issue of What's cooking report for this year. See you all in the new year. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * jc/t9001-modernise (2014-11-25) 5 commits (merged to 'next' on 2014-12-23 at 3a2ec87) + t9001: style modernisation phase #5 + t9001: style modernisation phase #4 + t9001: style modernisation phase #3 + t9001: style modernisation phase #2 + t9001: style modernisation phase #1 * mh/update-ref-verify (2014-12-11) 2 commits (merged to 'next' on 2014-12-23 at 3aa9a62) + update-ref: fix verify command with missing oldvalue + t1400: add some more tests of update-ref --stdin's verify command git update-ref --stdin's verify command did not work well when oldvalue, which is documented as optional, was missing. -- [New Topics] * bw/maint-0090-awk-tweak (2014-12-23) 1 commit (merged to 'next' on 2014-12-29 at 9301c36) + t0090: tweak awk statement for Solaris /usr/xpg4/bin/awk Will merge to 'master'. * cc/bisect-rev-parsing (2014-12-29) 2 commits - bisect: add test to check that revs are properly parsed - bisect: parse revs before passing them to check_expected_revs() The logic in git bisect bad HEAD etc. to avoid forcing the test of the common ancestor of bad and good commits was broken. Will merge to 'next'. * es/checkout-index-temp (2014-12-29) 5 commits - checkout-index: fix --temp relative path mangling - t2004: demonstrate broken relative path printing - t2004: standardize file naming in symlink test - t2004: drop unnecessary write-tree/read-tree - t2004: modernize style git checkout-index --temp=$target $path did not work correctly for paths outside the current subdirectory in the project. Will merge to 'next'. * js/remote-add-with-insteadof (2014-12-23) 2 commits - Add a regression test for 'git remote add existing same-url' - git remote: allow adding remotes agreeing with url.insteadOf git remote add $name $URL is now allowed when url.$URL.insteadOf is already defined. * rs/plug-strbuf-leak-in-lock-ref (2014-12-29) 1 commit - refs: plug strbuf leak in lock_ref_sha1_basic() Will merge to 'next'. * rs/plug-strbuf-leak-in-merge (2014-12-29) 1 commit - merge: release strbuf after use in suggest_conflicts() Will merge to 'next'. * rs/simplify-parsing-commit-tree-S (2014-12-29) 1 commit - commit-tree: simplify parsing of option -S using skip_prefix() Will merge to 'next'. * rs/simplify-transport-get (2014-12-29) 1 commit - transport: simplify duplicating a substring in transport_get() using xmemdupz() Will merge to 'next'. * sb/doc-submitting-patches-keep-notes (2014-12-29) 1 commit - Documentation/SubmittingPatches: Explain the rationale of git notes Will merge to 'next'. -- [Stalled] * pw/remote-set-url-fetch (2014-11-26) 1 commit - remote: add --fetch and --both options to set-url Expecting a reroll. * ms/submodule-update-config-doc (2014-11-03) 1 commit - submodule: clarify documentation for update subcommand Needs a reroll ($gmane/259037). * je/quiltimport-no-fuzz (2014-10-21) 2 commits - git-quiltimport: flip the default not to allow fuzz - git-quiltimport.sh: allow declining fuzz with --exact option quiltimport drove git apply always with -C1 option to reduce context of the patch in order to give more chance to somewhat stale patches to apply. Add an --exact option to disable, and also -C$n option to customize this behaviour. The top patch optionally flips the default to --exact. Tired of waiting for an Ack; will discard. * jc/push-cert-hmac-optim (2014-09-25) 2 commits - receive-pack: truncate hmac early and convert only necessary bytes - sha1_to_hex: split out hex-format n bytes helper and use it This is we could do this if we wanted to, not we measured and it improves performance critical codepath. Will perhaps drop. * mt/patch-id-stable (2014-06-10) 1 commit - patch-id: change default to stable Teaches git patch-id to compute the patch ID that does not change when the files in a single patch is reordered. As this new algorithm is backward incompatible, the last bit to flip it to be the default is left out of 'master' for now. Nobody seems to be jumping up down requesting this last step, which makes the result somewhat backward incompatible. Will perhaps drop. * tr/remerge-diff (2014-11-10) 9 commits - t4213: avoid | in sed regexp - log --remerge-diff: show what the conflict resolution changed - name-hash: allow dir hashing even when
[PATCHv7 2/9] send-pack: Rename ref_update_to_be_sent to check_to_send_update
This renames ref_update_to_be_sent to check_to_send_update and inverts
the meaning of the return value. Having the return value inverted we
can have different values for the error codes. This is useful in a
later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED
case.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
This was introduced with the [PATCHv2] series.
Changes v2 - v3:
* Rename to check_to_send_update
* Negative error values.
* errors values are #define'd and not raw numbers.
skipped v4 v5
v6:
* negative #define'd values
v7:
* no changes
send-pack.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 6d0c159..b7d8e01 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf
*sb)
for_each_commit_graft(advertise_shallow_grafts_cb, sb);
}
-static int ref_update_to_be_sent(const struct ref *ref, const struct
send_pack_args *args)
+#define CHECK_REF_NO_PUSH -1
+#define CHECK_REF_STATUS_REJECTED -2
+#define CHECK_REF_UPTODATE -3
+static int check_to_send_update(const struct ref *ref, const struct
send_pack_args *args)
{
if (!ref-peer_ref !args-send_mirror)
- return 0;
+ return CHECK_REF_NO_PUSH;
/* Check for statuses set by set_ref_status_for_push() */
switch (ref-status) {
@@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref,
const struct send_pack_a
case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_REJECT_STALE:
case REF_STATUS_REJECT_NODELETE:
+ return CHECK_REF_STATUS_REJECTED;
case REF_STATUS_UPTODATE:
- return 0;
+ return CHECK_REF_UPTODATE;
default:
- return 1;
+ return 0;
}
}
@@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,
strbuf_addstr(cert, \n);
for (ref = remote_refs; ref; ref = ref-next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
update_seen = 1;
strbuf_addf(cert, %s %s %s\n,
@@ -369,7 +373,7 @@ int send_pack(struct send_pack_args *args,
* the pack data.
*/
for (ref = remote_refs; ref; ref = ref-next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
if (!ref-deletion)
@@ -390,7 +394,7 @@ int send_pack(struct send_pack_args *args,
if (args-dry_run || args-push_cert)
continue;
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
old_hex = sha1_to_hex(ref-old_sha1);
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 5/9] receive-pack.c: move transaction handling in a central place
No functional changes intended.
This moves all code related to transactions into the execute_commands_loop
function which was factored out of execute_commands. This includes
beginning and committing the transaction as well as dealing with the
errors which may occur during the begin and commit phase of a transaction.
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v7:
new in v7, this is part of the previous [PATCH 4/7]
receive-pack.c: receive-pack.c: use a single ref_transaction for atomic
pushes
This covers the suggestion of patch 2 and 3 by Eric
patch 2: Factor out the main 'for' loop of execute_commands() into a
new function. This new function will eventually become
execute_commands_non_atomic(). At this point, execute_commands() is
pretty much in its final form with the exception of the upcoming 'if
(use_atomic)' conditional.
patch 3: Morph the function extracted in patch 2 into
execute_commands_non_atomic() by adding transaction handling inside
the 'for' loop (and applying the changes from the early part of the
patch which go along with that).
builtin/receive-pack.c | 62 ++
1 file changed, 47 insertions(+), 15 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 06eb287..915ad3c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -67,6 +67,7 @@ static const char *NONCE_SLOP = SLOP;
static const char *nonce_status;
static long nonce_stamp_slop;
static unsigned long nonce_stamp_slop_limit;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -822,6 +823,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
}
if (is_null_sha1(new_sha1)) {
+ struct strbuf err = STRBUF_INIT;
if (!parse_object(old_sha1)) {
old_sha1 = NULL;
if (ref_exists(name)) {
@@ -831,35 +833,36 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
cmd-did_not_exist = 1;
}
}
- if (delete_ref(namespaced_name, old_sha1, 0)) {
- rp_error(failed to delete %s, name);
+ if (ref_transaction_delete(transaction,
+ namespaced_name,
+ old_sha1,
+ 0, old_sha1 != NULL,
+ push, err)) {
+ rp_error(%s, err.buf);
+ strbuf_release(err);
return failed to delete;
}
+ strbuf_release(err);
return NULL; /* good */
}
else {
struct strbuf err = STRBUF_INIT;
- struct ref_transaction *transaction;
-
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
return shallow error;
- transaction = ref_transaction_begin(err);
- if (!transaction ||
- ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, push,
- err) ||
- ref_transaction_commit(transaction, err)) {
- ref_transaction_free(transaction);
-
+ if (ref_transaction_update(transaction,
+ namespaced_name,
+ new_sha1, old_sha1,
+ 0, 1, push,
+ err)) {
rp_error(%s, err.buf);
strbuf_release(err);
+
return failed to update ref;
}
-
- ref_transaction_free(transaction);
strbuf_release(err);
+
return NULL; /* good */
}
}
@@ -1107,13 +1110,42 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
+ execute_commands_loop(commands, si);
+
+ check_shallow_bugs(commands, si);
+}
+
+static void execute_commands_loop(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ struct strbuf err = STRBUF_INIT;
+
for (cmd = commands; cmd; cmd = cmd-next) {
if (!should_process_cmd(cmd))
continue;
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ rp_error(%s,
[PATCHv7 7/9] receive-pack.c: enable atomic push protocol support
This enables the atomic protocol option as implemented in the previous patches. Signed-off-by: Stefan Beller sbel...@google.com --- Notes: v7: * new with v7 of the patch series. * this was part of the first patch in the series, moved back here for bisectability builtin/receive-pack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 7e3bbe6..6da6975 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -173,7 +173,8 @@ static void show_ref(const char *path, const unsigned char *sha1) struct strbuf cap = STRBUF_INIT; strbuf_addstr(cap, - report-status delete-refs side-band-64k quiet); + report-status delete-refs side-band-64k quiet + atomic); if (prefer_ofs_delta) strbuf_addstr(cap, ofs-delta); if (push_cert_nonce) -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes
This adds tests for the atomic push option.
The first four tests check if the atomic option works in
good conditions and the last three patches check if the atomic
option prevents any change to be pushed if just one ref cannot
be updated.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v7: no changes
v6: the same as v2, so I can resend the whole series as v6
v3 v4 v5 were skipped
Changes v1 - v2:
Please drop unused comments; they are distracting.
ok
It is not wrong per-se, but haven't you already tested in
combination with --mirror in the previous test?
I fixed the previous tests, so that there is no --mirror
and --atomic together. There is still a first --mirror push
for setup and a second with --atomic branchnames though
check_branches upstream master HEAD@{2} second HEAD~
A similar function test_ref_upstream is introduced.
What's the value of this test? Isn't it a non-fast-forward check
you already tested in the previous one?
I messed up there. Originally I wanted to test the 2 different
stages of rejection. A non-fast-forward check is done locally and
we don't even try pushing. But I also want to test if we locally
thing all is good, but the server refuses a ref to update.
This is now done with the last test named 'atomic push obeys
update hook preventing a branch to be pushed'. And that still fails.
I'll investigate that, while still sending out the series for another
review though.
* Redone the test helper, there is test_ref_upstream now.
This tests explicitely for SHA1 values of the ref.
(It's needed in the last test for example. The git push fails,
but still modifies the ref :/ )
* checked all chains and repaired them
* sometimes make use of git -C workdir
Notes v1:
Originally Ronnie had a similar patch prepared. But as I added
more tests and cleaned up the existing tests (e.g. use test_commit
instead of echo one file gitadd file git commit -a -m 'one',
removal of dead code), the file has changed so much that I'd rather
take ownership.
t/t5543-atomic-push.sh | 178 +
1 file changed, 178 insertions(+)
create mode 100755 t/t5543-atomic-push.sh
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
new file mode 100755
index 000..b81a542
--- /dev/null
+++ b/t/t5543-atomic-push.sh
@@ -0,0 +1,178 @@
+#!/bin/sh
+
+test_description='pushing to a repository using the atomic push option'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf workbench upstream
+ test_create_repo upstream
+ test_create_repo workbench
+ (
+ cd upstream
+ git config receive.denyCurrentBranch warn
+ )
+ (
+ cd workbench
+ git remote add up ../upstream
+ )
+}
+
+# Compare the ref ($1) in upstream with a ref value from workbench ($2)
+# i.e. test_refs second HEAD@{2}
+test_refs () {
+ test $# = 2
+ git -C upstream rev-parse --verify $1 expect
+ git -C workbench rev-parse --verify $2 actual
+ test_cmp expect actual
+}
+
+test_expect_success 'atomic push works for a single branch' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git push --mirror up
+ test_commit two
+ git push --atomic up master
+ )
+ test_refs master master
+'
+
+test_expect_success 'atomic push works for two branches' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git branch second
+ git push --mirror up
+ test_commit two
+ git checkout second
+ test_commit three
+ git push --atomic up master second
+ )
+ test_refs master master
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --mirror' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git checkout -b second
+ test_commit two
+ git push --atomic --mirror up
+ )
+ test_refs master master
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --force' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git branch second master
+ test_commit two_a
+ git checkout second
+ test_commit two_b
+ test_commit three_b
+ test_commit four
+ git push --mirror up
+ # The actual test is below
+ git checkout master
+ test_commit three_a
+ git checkout second
[PATCHv7 6/9] receive-pack.c: add execute_commands_atomic function
Update receive-pack to use an atomic transaction iff the client negotiated
that it wanted atomic push. This leaves the default behavior to be the old
non-atomic one ref at a time update. This is to cause as little disruption
as possible to existing clients. It is unknown if there are client scripts
that depend on the old non-atomic behavior so we make it opt-in for now.
If it turns out over time that there are no client scripts that depend on the
old behavior we can change git to default to use atomic pushes and instead
offer an opt-out argument for people that do not want atomic pushes.
Inspired-by: Ronnie Sahlberg sahlb...@google.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Changes in v7:
Eric suggested to replace [PATCH 4/7] receive-pack.c:
receive-pack.c: use a single ref_transaction for atomic pushes
by smaller patches
This is the last patch replacing said large commit.
Changes in v6:
This is a complete rewrite of the patch essentially.
Eric suggested to split up the flow into functions, so
it is easier to read. It's more lines of code, but indeed
it's easier to follow. Thanks Eric!
Note there is another patch following this one
moving the helper functions above execute_commands.
I just choose the order of the functions in this patch
to have a better diff (just inserting the call to the function
execute_commands_non_atomic and that function directly follows.)
The next patch of the series will move that up.
Because of the rewrite and the fixes of the previous five
versions there is not much left of Ronnies original patch,
so I'll claim authorship of this one.
Changes v1 - v2:
* update(...) assumes to be always in a transaction
* Caring about when to begin/commit transactions is put
into execute_commands
v2-v3:
* meditated about the error flow. Now we always construct a local
strbuf err if required. Then the flow is easier to follow and
destruction of it is performed nearby.
* early return in execute_commands if transaction_begin fails.
v3-v4:
* revamp logic again. This should keep the non atomic behavior
as is (in case of error say so, in non error case just free the
transaction). In the atomic case we either do nothing (when no error),
or abort with the goto.
if (!cmd-error_string) {
if (!use_atomic
ref_transaction_commit(transaction, err)) {
ref_transaction_free(transaction);
rp_error(%s, err.buf);
strbuf_release(err);
cmd-error_string = failed to update ref;
}
} else if (use_atomic) {
goto atomic_failure;
} else {
ref_transaction_free(transaction);
}
* Having the goto directly there when checking for cmd-error_string,
we don't need to do it again, so the paragraph explaining the error
checking is gone as well. (Previous patch had the following, this is
put at the end of the function, where the goto jumps to and the
comment
has been dropped.
+ /*
+* update(...) may abort early (i.e. because the hook refused to
+* update that ref) which then doesn't even record a transaction
+* regarding that ref. Make sure all commands are without error
+* and then commit atomically.
+*/
+ for (cmd = commands; cmd; cmd = cmd-next)
+ if (cmd-error_string)
+ break;
v4-v5:
Eric wrote:
Repeating from my earlier review[1]: If the 'pre-receive' hook
declines, then this transaction is left dangling (and its resources
leaked).
You're right. The initialization of the transaction is now
near the actual loop after the pre receive hook.
The !use_atomic case (below), calls this error failed to start
transaction, not merely transaction error.
ok, now both are transaction failed to start.
In all cases where these generic errors are reported,
we do have a rp_error(...) with details.
Furthermore, in the use_atomic case (also below), when a commit fails,
you assign err.buf to cmd-error_string rather than a generic
transaction error message. What differs between these cases which
makes the generic message preferable here over the more specific
err.buf message?
They are the same now.
Repeating from my earlier review[1]: This is leaking
[PATCHv7 4/9] receive-pack.c: simplify execute_commands
No functional changes intended.
This commit shortens execute_commands by moving some parts of the code
to extra functions.
Suggested-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v7:
new in v7 as in v7 I'd split up the previous
[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction
for atomic pushes
as suggested by Eric.
This is pretty much
patch 1: Factor out code into helper functions which will be needed by
the upcoming atomic and non-atomic worker functions. Example helpers:
'cmd-error_string' and cmd-skip_update' check; and the
'si-shallow_ref[cmd-index]' check and handling.
builtin/receive-pack.c | 49 -
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4e8eaf7..06eb287 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command
*commands)
}
}
+static int should_process_cmd(struct command *cmd)
+{
+ if (cmd-error_string)
+ return 0;
+ if (cmd-skip_update)
+ return 0;
+ return 1;
+}
+
+static void check_shallow_bugs(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ if (shallow_update si-shallow_ref[cmd-index]) {
+ error(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (shallow_update !checked_connectivity)
+ error(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd-next) {
- if (cmd-error_string)
- continue;
-
- if (cmd-skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd-error_string = update(cmd, si);
- if (shallow_update !cmd-error_string
- si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
- checked_connectivity = 0;
- }
}
-
- if (shallow_update !checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ check_shallow_bugs(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 0/9] atomic pushes
The patch [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes was dropped and redone as 3 separate patches. This wasn't just done for doing it, but the end result has also changed. We have more smaller functions doing one thing instead of these larger functions. Thanks for the ideas, Eric! Also the advertisement of the atomic capabilites was moved to a later new patch in this series. This helps when you want to bisect this series later. Thanks Michael for pointing this out! Thanks, Stefan Ronnie Sahlberg (3): receive-pack.c: add documentation for atomic push support send-pack.c: add --atomic command line argument push.c: add an --atomic argument Stefan Beller (6): send-pack: Rename ref_update_to_be_sent to check_to_send_update receive-pack.c: simplify execute_commands receive-pack.c: move transaction handling in a central place receive-pack.c: add execute_commands_atomic function receive-pack.c: enable atomic push protocol support t5543-atomic-push.sh: add basic tests for atomic pushes Documentation/git-push.txt| 7 +- Documentation/git-send-pack.txt | 7 +- Documentation/technical/protocol-capabilities.txt | 13 +- builtin/push.c| 5 + builtin/receive-pack.c| 154 +++ builtin/send-pack.c | 6 +- remote.h | 3 +- send-pack.c | 65 +++- send-pack.h | 3 +- t/t5543-atomic-push.sh| 178 ++ transport.c | 5 + transport.h | 1 + 12 files changed, 404 insertions(+), 43 deletions(-) create mode 100755 t/t5543-atomic-push.sh -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 1/9] receive-pack.c: add documentation for atomic push support
From: Ronnie Sahlberg sahlb...@google.com
This documents the protocol option between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it. The capability is also not yet advertised
by receive-pack as git doesn't know how to handle it yet.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Changes v1 - v2:
* Name it atomic instead of atomic-push. The name changes affects
the flags send over the wire as well as the flags in
struct send_pack_args
* Add check, which was part of the later patch here:
if (args-atomic !atomic_supported) {
fprintf(stderr, Server does not support atomic push.);
return -1;
}
v2 - v3:
More concise error reporting as suggested by Junio
- fprintf(stderr, Server does not support atomic push.);
- return -1;
+ return error(server does not support atomic push.);
skipped v4 v5
v6:
* s/one single atomic transaction./one atomic transaction./
* die(_( instead of return error(
v7:
* Don't advertise the atomic push capability yet. We cannot handle it at
this point.
* reword commit message
Documentation/technical/protocol-capabilities.txt | 13 +++--
builtin/receive-pack.c| 3 +++
send-pack.c | 10 ++
send-pack.h | 3 ++-
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..4f8a7bf 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client
requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress
server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic
+--
+
+If the server sends the 'atomic' capability it is capable of accepting
+atomic pushes. If the pushing client requests this capability, the server
+will update the refs in one atomic transaction. Either all refs are
+updated or none.
+
allow-tip-sha1-in-want
--
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..4e8eaf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -1179,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array
*shallow)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, quiet))
quiet = 1;
+ if (parse_feature_request(feature_list, atomic))
+ use_atomic = 1;
}
if (!strcmp(line, push-cert)) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..6d0c159 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int use_atomic = 0;
+ int atomic_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,
agent_supported = 1;
if (server_supports(no-thin))
args-use_thin_pack = 0;
+ if (server_supports(atomic))
+ atomic_supported = 1;
if (args-push_cert) {
int len;
@@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
Perhaps
[PATCHv7 8/9] push.c: add an --atomic argument
From: Ronnie Sahlberg sahlb...@google.com
Add a command line argument to the git push command to request atomic
pushes.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Changes v1 - v2
It's --atomic now! (dropping the -push)
v2-v3:
* s/atomic-push/atomic/
* s/the an/an/
* no serverside, but just remote instead
* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH
skipped v4 v5
v6:
- OPT_BIT(0, atomic, flags, N_(use an atomic transaction
remote),
+ OPT_BIT(0, atomic, flags, N_(request atomic transaction on
remote side),
v7:
Use OPT_BOOL instead of OPT_BIT.
This allows for --no-atomic option on the command line.
Documentation/git-push.txt | 7 ++-
builtin/push.c | 5 +
transport.c| 1 +
transport.h| 1 +
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21b3f29..4764fcf 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
SYNOPSIS
[verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run]
[--receive-pack=git-receive-pack]
+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n |
--dry-run] [--receive-pack=git-receive-pack]
[--repo=repository] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream] [--signed]
[--force-with-lease[=refname[:expect]]]
@@ -136,6 +136,11 @@ already exists on the remote side.
logged. See linkgit:git-receive-pack[1] for the details
on the receiving end.
+--[no-]atomic::
+ Use an atomic transaction on the remote side if available.
+ Either all refs are updated, or on error, no refs are updated.
+ If the server does not support atomic pushes the push will fail.
+
--receive-pack=git-receive-pack::
--exec=git-receive-pack::
Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..8f1d945 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -487,6 +487,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
int flags = 0;
int tags = 0;
int rc;
+ int atomic = 0;
const char *repo = NULL;/* default repository */
struct option options[] = {
OPT__VERBOSITY(verbosity),
@@ -518,6 +519,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
OPT_BIT(0, follow-tags, flags, N_(push missing but relevant
tags),
TRANSPORT_PUSH_FOLLOW_TAGS),
OPT_BIT(0, signed, flags, N_(GPG sign the push),
TRANSPORT_PUSH_CERT),
+ OPT_BOOL(0, atomic, atomic, N_(request atomic transaction
on remote side)),
OPT_END()
};
@@ -533,6 +535,9 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
if (tags)
add_refspec(refs/tags/*);
+ if (atomic)
+ flags |= TRANSPORT_PUSH_ATOMIC;
+
if (argc 0) {
repo = argv[0];
set_refspecs(argv + 1, argc - 1, repo);
diff --git a/transport.c b/transport.c
index c67feee..1373152 100644
--- a/transport.c
+++ b/transport.c
@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport,
struct ref *remote_re
args.dry_run = !!(flags TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags TRANSPORT_PUSH_PORCELAIN);
args.push_cert = !!(flags TRANSPORT_PUSH_CERT);
+ args.atomic = !!(flags TRANSPORT_PUSH_ATOMIC);
args.url = transport-url;
ret = send_pack(args, data-fd, data-conn, remote_refs,
diff --git a/transport.h b/transport.h
index 3e0091e..18d2cf8 100644
--- a/transport.h
+++ b/transport.h
@@ -125,6 +125,7 @@ struct transport {
#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
#define TRANSPORT_PUSH_CERT 2048
+#define TRANSPORT_PUSH_ATOMIC 4096
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) -
gettext_width(x)), (x)
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv7 3/9] send-pack.c: add --atomic command line argument
From: Ronnie Sahlberg sahlb...@google.com
This adds support to send-pack to negotiate and use atomic pushes
iff the server supports it. Atomic pushes are activated by a new command
line flag --atomic.
In order to do this we also need to change the semantics for send_pack()
slightly. The existing send_pack() function actually doesn't send all the
refs back to the server when multiple refs are involved, for example
when using --all. Several of the failure modes for pushes can already be
detected locally in the send_pack client based on the information from the
initial server side list of all the refs as generated by receive-pack.
Any such refs that we thus know would fail to push are thus pruned from
the list of refs we send to the server to update.
For atomic pushes, we have to deal thus with both failures that are detected
locally as well as failures that are reported back from the server. In order
to do so we treat all local failures as push failures too.
We introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can
flag all refs that we would normally have tried to push to the server
but we did not due to local failures. This is to improve the error message
back to the end user to flag that these refs failed to update since the
atomic push operation failed.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Notes:
Changes v1 - v2:
* Now we only need a very small change in the existing code and have
a new static function, which cares about error reporting.
Junio wrote:
Hmph. Is atomic push so special that it deserves a separate
parameter? When we come up with yet another mode of failure,
would
we add another parameter to the callers to this function?
* error messages are worded differently (lower case!),
* use of error function instead of fprintf
* undashed the printed error message (atomic push failed);
Changes v2 - v3:
We avoid assignment inside a conditional.
Ok I switched to using a switch statement.
skipped v4 v5
v6:
* realign to one error string:
+ error(atomic push failed for ref %s. status: %d\n,
+ failing_ref-name, failing_ref-status);
* Use correct value now (negative defined from previous patch)
v7:
* return error(...); instead of error(...); return -1;
Documentation/git-send-pack.txt | 7 ++-
builtin/send-pack.c | 6 +-
remote.h| 3 ++-
send-pack.c | 39 +--
transport.c | 4
5 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..45c7725 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another
repository
SYNOPSIS
[verse]
-'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]
+'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic]
[host:]directory [ref...]
DESCRIPTION
---
@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush
packet.
Send a thin pack, which records objects in deltified form based
on objects not included in the pack to reduce network traffic.
+--atomic::
+ Use an atomic transaction for updating the refs. If any of the refs
+ fails to update then the entire push will fail without changing any
+ refs.
+
host::
A remote host to house the repository. When this
part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..b961e5a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
#include sha1-array.h
static const char send_pack_usage[] =
-git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]\n
+git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic]
[host:]directory [ref...]\n
--all and explicit ref specification are mutually exclusive.;
static struct send_pack_args args;
@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char
*prefix)
args.use_thin_pack = 1;
continue;
}
+ if (!strcmp(arg, --atomic)) {
+ args.atomic = 1;
+ continue;
+ }
if (!strcmp(arg, --stateless-rpc)) {
Re: [PATCHv7 0/9] atomic pushes
please ignore this series. I'm sorry for the noise. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's Perl scripts can fail if user is configured for perlbrew
On 12/29/14, 7:40 AM, Torsten Bögershausen wrote: Having problems with different perl installations is not an unknown problem in Git, I would say. And Git itself is prepared to handle this situation: In Makefile I can read: # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl). (What Git can not decide is which perl it should use, the one pointed out by $PATH or /usr/bin/perl.) What does type perl say ? And what happens when you build and install Git like this: PERL_PATH=/XX/YY/perl make install --- Are you thinking about changing ifndef PERL_PATH PERL_PATH = /usr/bin/perl endif -- into -- ifndef PERL_PATH PERL_PATH = $(shell which perl) endif --- At first glance that could make sense, at least to me. The problem in this case is the Perl being used at run-time, not build-time. The building of git is done by the homebrew project in this case, so I don't have direct control over it. Randy -- Randy J. Ray Sunnyvale, CA http://www.dereferenced.com rj...@blackperl.com twitter.com/rjray Silicon Valley Scale Modelers: http://www.svsm.org -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@tuxfamily.org writes:
When running for example git bisect bad HEAD or
git bisect good master, the parameter passed to
git bisect (bad|good) has to be parsed into a
commit hash before checking if it is the expected
commit or not.
Hmm, is that because you wrote commit object name in 40-hex in the
EXPECTED_REV and you need to compare with what the user gave you
which could be symbolic?
Yes, that's the reason.
The conversion makes sense, but why is it a bad thing to say
git bisect bad maint
when 'maint' is not what you checked out in the current bisect run
in the first place (perhaps you checked if it is good or bad manually
before you started bisecting)?
It is not a bad thing to test another commit than the one that has
been checked out and then to say if it is good or bad. But if you
do that then it is safer to check if a merge base should be tested.
I can discuss this point further and there are indeed some
optimisations we could implement in this area, but I think it is
better to try to just fix the bug first.
diff --git a/git-bisect.sh b/git-bisect.sh
index 6cda2b5..2fc07ac 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,15 +237,18 @@ bisect_state() {
check_expected_revs $rev ;;
2,bad|*,good|*,skip)
This part accepts arbitrary number of revs when running good and
skip, e.g.
git bisect good maint master next
and it loops
Yeah.
shift
-eval=''
+hash_list=''
for rev in $@
...
+for rev in $hash_list
+do
+bisect_write $state $rev
+done
+check_expected_revs $hash_list ;;
But check_expected_revs loops and leaves the loop early when it
finds anything that is not expected.
... goes and looks ...
Hmph, I think the logic in check_expected_revs is not wrong, but
this helper function is grossly misnamed. It is not checking and
rejecting the user input---it is checking to see if it can bypass
check_good_are_ancestors_of_bad() which is expensive, so when it
sees any one of the input is not what it checked out, it just
disables the optimization.
Yeah, that's the idea. If you have a better name for
check_expected_revs(), I can change it in another patch.
And yeah, check_good_are_ancestors_of_bad() is expensive to compute
and also expensive because it might mean that more tests have to be
performed by the user to be safe.
OK, will queue.
Thanks,
Christian.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-rebase documentation: explain the exit code of custom commands
Signed-off-by: Stefan Beller sbel...@google.com --- Documentation/git-rebase.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 924827d..ffadb0b 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -372,7 +372,8 @@ idea unless you know what you are doing (see BUGS below). --exec cmd:: Append exec cmd after each line creating a commit in the final history. cmd will be interpreted as one or more shell - commands. + commands. Rebasing will stop for manual inspection if the command + returns a non zero exit code. + This option can only be used with the `--interactive` option (see INTERACTIVE MODE below). -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git update-ref --stdin : too many open files
On Sun, Dec 28, 2014 at 5:28 PM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm doing some work in this area, so I should be able to work on the bugfix in the not-too-distant future. My feeling is that the bug is unlikely to affect many current users, though it definitely should be fixed before sb/atomic-push is merged. So are you heading for the bug fix? I'd then drop this off my todo list. Thanks, Stefan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's Perl scripts can fail if user is configured for perlbrew
On 12/29/14, 7:21 AM, Ævar Arnfjörð Bjarmason wrote: [CC'd the perlbrew author] This is a bit of a tricky issue. Using whatever perl is defined in the environment is just as likely to break, in general the build process tries to pick these assets at compile-time. Imagine you're experimenting with some custom perl version and now Git inexplicably breaks. It's better if Git detects a working perl when you compile it and sticks with that, which is why we use /usr/bin/perl by default. With perl being an external dependency, sticking with whatever at the compile time basically means these should stick: - `which perl`, and the same $Config options - Every module contained in PERL5LIB - Other external executable dependencies of some .pm files that lives somewhere in PATH We could of course build an app bundle dir like a lightweight container. to mitigate this... but that would'nt be usefull without tweaking the shebang line of the scripts -- to point to the perl script (shim or real) that should be compatible with the newly built git. I'd argue that trying to compile git (or other stuff in general) against a perlbrew-managed perl is something that perlbrew executable cannot manage. Because it is both valid that the user is doing this intentionally and want the outcome, or the user is doing this unintentionally. And even if we have a shim perl script, it would'nt help as long as it is the whatever perl in PATH -- which might just be incompitble. I wonder if disabling perlbrew per-building can deal with this: 'perlbrew off' If this end up changing the shebang line then maybe, if not then it'll still intefere after perlbrew is re-activated. The other option to minimize perlbrew interferince is of course to completely deprecate the global env var approach and make them project local. But that's another story. -- Cheers, Kang-min Liu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's Perl scripts can fail if user is configured for perlbrew
On Mon, Dec 29, 2014 at 10:57 PM, Randy J. Ray rj...@blackperl.com wrote: On 12/29/14, 7:40 AM, Torsten Bögershausen wrote: Having problems with different perl installations is not an unknown problem in Git, I would say. And Git itself is prepared to handle this situation: In Makefile I can read: # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl). (What Git can not decide is which perl it should use, the one pointed out by $PATH or /usr/bin/perl.) What does type perl say ? And what happens when you build and install Git like this: PERL_PATH=/XX/YY/perl make install --- Are you thinking about changing ifndef PERL_PATH PERL_PATH = /usr/bin/perl endif -- into -- ifndef PERL_PATH PERL_PATH = $(shell which perl) endif --- At first glance that could make sense, at least to me. The problem in this case is the Perl being used at run-time, not build-time. The building of git is done by the homebrew project in this case, so I don't have direct control over it. Correct, but we don't change /usr/bin/perl at runtime, we hardcode that at compile-time. Similarly we could hardcode PERL5LIB at compile-time, but we don't, if we did you wouldn't have this problem. I.e. the problem is that we're using the system-provided perl with a custom PERL5LIB set for the benefit of a non-system provided perl installed after you built Git (or built in a different environment...) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] Documentation/SubmittingPatches: Explain the rationale of git notes
On Mon, Dec 29, 2014 at 12:42 PM, Stefan Beller sbel...@google.com wrote: This adds more explanation of why you want to have the --notes option given to git format-patch. Signed-off-by: Stefan Beller sbel...@google.com --- diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e3c942e..f42c607 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -177,7 +177,12 @@ message starts, you can put a From: line to name that person. You often want to add additional explanation about the patch, other than the commit message itself. Place such cover letter material between the three dash lines and the diffstat. Git-notes -can also be inserted using the `--notes` option. +can also be inserted using the `--notes` option. If you are one +of those developers who cannot write perfect code the first time +and need multiple iterations of review and discussion, you may +want to keep track of the changes between different versions of +a patch using notes and then also use the `--notes` option when +preparing the patch for submission. Perhaps this could be rephrased in a less derogatory fashion like this: ...material between the three dash line and the diffstat. For patches requiring multiple iterations of review and discussion, an explanation of changes between each iteration can be kept in Git-notes and inserted automatically following the three dash line via `git format-patch --notes`. Do not attach the patch as a MIME attachment, compressed or not. Do not let your e-mail client send quoted-printable. Do not let -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] Documentation/SubmittingPatches: Explain the rationale of git notes
On Mon, Dec 29, 2014 at 3:18 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Dec 29, 2014 at 12:42 PM, Stefan Beller sbel...@google.com wrote: This adds more explanation of why you want to have the --notes option given to git format-patch. Signed-off-by: Stefan Beller sbel...@google.com --- diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e3c942e..f42c607 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -177,7 +177,12 @@ message starts, you can put a From: line to name that person. You often want to add additional explanation about the patch, other than the commit message itself. Place such cover letter material between the three dash lines and the diffstat. Git-notes -can also be inserted using the `--notes` option. +can also be inserted using the `--notes` option. If you are one +of those developers who cannot write perfect code the first time +and need multiple iterations of review and discussion, you may +want to keep track of the changes between different versions of +a patch using notes and then also use the `--notes` option when +preparing the patch for submission. Perhaps this could be rephrased in a less derogatory fashion like this: This wasn't meant to be derogatory at all. It was more of subtle humor. Sorry for the confusion. If this is offensive in any way I'd rather go with your suggestion indeed. ...material between the three dash line and the diffstat. For patches requiring multiple iterations of review and discussion, an explanation of changes between each iteration can be kept in Git-notes and inserted automatically following the three dash line via `git format-patch --notes`. Do not attach the patch as a MIME attachment, compressed or not. Do not let your e-mail client send quoted-printable. Do not let -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Dec 2014, #05; Mon, 29)
From: Junio C Hamano gits...@pobox.com * po/doc-core-ignorestat (2014-12-12) 1 commit (merged to 'next' on 2014-12-23 at d2b3e84) + doc: core.ignoreStat clarify the --assume-unchanged effect Will merge to 'master'. I was hoping to re-roll but family Christmas / New Year visits have disrupted plans. Could you hold for another week or so? (though happy either way) -- Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
About my git workflow; maybe it's useful for others
Hi, so I have been sending commits to the git mailing list occasionally for quite some time. In the last couple of weeks I send more and more patches to the mailing list as it's part of my job now. Here is a collection of practices I am following (or want to follow) and they seem to be effective. Most of this is already documented in various documents in Documentation/* and this email is no news for the regular contributors. It may help new comers though. * Split patches up into logical pieces as you go. It's easy to go wild and after hours of hacking you have implemented a cool new feature. This the natural flow of hacking as it's the most fun. But this approach is not easy to be reviewed. So let me explain how reviewing works in the git project. Reviewing works in the git project is quite liberal, anybody is encouraged to comment on patches flying by. Junio, the maintainer then decides which patches he picks up and merges them into the various stages of git (pu, next, master, maint). The decision which patches are worth for inclusion is based on the amount of discussion by the community and generally a patch only makes it if a concensus is met. * git send-email is the only email client I trust for sending patches While mail clients such as Thunderbird or the gmail interface are optimized to be used by everyday people it behaves differently than you would expect. For example these mail clients may convert tabs to white spaces depending on the configuration. The default configuration is usually not sane. To avoid that I tend to use git send-email only when sending patches to the list. Here is my setup: git send-email needs a SMTP client to talk to a server, as I am using Ubuntu I need to apt-get install msmtp. Then there is a configuration file .msmtprc which reads: defaults tls on # this may be different in other distributions: tls_trust_file /usr/share/ncat/ca-bundle.crt logfile ~/.msmtp.log account gmail host smtp.gmail.com port 587 from yourname@gmail.com auth on user yourname@gmail.com password yourpassword # Set a default account account default : gmail The git configuration for sending email via msmtp is git config --global sendemail.smtpserver /usr/bin/msmtp git config --global sendemail.smtpuser yourname git config --global sendemail.from yourname * Keep notes between different versions of your patch Look into the man page of git notes for the configuration variables. The only thing requried should be git config notes.rewriteRef refs/notes/* to enable keeping notes during a rebase. * Make sure you're not embarassed by the patches you send out I fail this talking point often. Write a patch or incorporate just a one line fix and send it off to the list. What could go wrong? One line fixes and small patches tend to look so easy that you forget to compile check or run the test suite, which then promptly breaks. To prevent such disasters, I want to do a: git rebase origin/master --exec=make --exec=make test before sending the patch series to the list. * Wait with sending out iterations of patches patience is a virtue I do not have So you successfully send your patches to the mailing list. By successful I both mean it worked technically as well as you got actually reviews and comments. Usually the comments are feedback how to improve the patch. Now it's easy to fall in the trap and fix all the problems the reviewers point out and resend the patches again to the list again. Waiting for roughly 24 hours helps a lot I found out. This brings you the following advantages: * You may get more comments from people in other time zones. Git contributors are quite international folks. So it would be impolite to not wait for everyone to at least have a look at it. They might be sleeping currently. * You will find yourself reviewing your patches twice. Once after fixing all the problems the review unveiled and once again before sending out the new iteration of the patch series. Stefan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
From: Ronnie Sahlberg sahlb...@google.com
This documents the protocol option between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it. The capability is also not yet advertised
by receive-pack as git doesn't know how to handle it yet.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Changes v1 - v2:
* Name it atomic instead of atomic-push. The name changes affects
the flags send over the wire as well as the flags in
struct send_pack_args
* Add check, which was part of the later patch here:
if (args-atomic !atomic_supported) {
fprintf(stderr, Server does not support atomic push.);
return -1;
}
v2 - v3:
More concise error reporting as suggested by Junio
- fprintf(stderr, Server does not support atomic push.);
- return -1;
+ return error(server does not support atomic push.);
skipped v4 v5
v6:
* s/one single atomic transaction./one atomic transaction./
* die(_( instead of return error(
v7:
* Don't advertise the atomic push capability yet. We cannot handle it at
this point.
* reword commit message
v8:
no changes
Documentation/technical/protocol-capabilities.txt | 13 +++--
builtin/receive-pack.c| 3 +++
send-pack.c | 10 ++
send-pack.h | 3 ++-
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..4f8a7bf 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client
requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress
server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic
+--
+
+If the server sends the 'atomic' capability it is capable of accepting
+atomic pushes. If the pushing client requests this capability, the server
+will update the refs in one atomic transaction. Either all refs are
+updated or none.
+
allow-tip-sha1-in-want
--
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..4e8eaf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -1179,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array
*shallow)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, quiet))
quiet = 1;
+ if (parse_feature_request(feature_list, atomic))
+ use_atomic = 1;
}
if (!strcmp(line, push-cert)) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..6d0c159 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int use_atomic = 0;
+ int atomic_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,
agent_supported = 1;
if (server_supports(no-thin))
args-use_thin_pack = 0;
+ if (server_supports(atomic))
+ atomic_supported = 1;
if (args-push_cert) {
int len;
@@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
[PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
No functional changes intended.
This moves all code related to transactions into the execute_commands_loop
function which was factored out of execute_commands. This includes
beginning and committing the transaction as well as dealing with the
errors which may occur during the begin and commit phase of a transaction.
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v8:
move execute_commands_loop before execute_commands, so it compiles/links
without warnings.
v7:
new in v7, this is part of the previous [PATCH 4/7]
receive-pack.c: receive-pack.c: use a single ref_transaction for atomic
pushes
This covers the suggestion of patch 2 and 3 by Eric
patch 2: Factor out the main 'for' loop of execute_commands() into a
new function. This new function will eventually become
execute_commands_non_atomic(). At this point, execute_commands() is
pretty much in its final form with the exception of the upcoming 'if
(use_atomic)' conditional.
patch 3: Morph the function extracted in patch 2 into
execute_commands_non_atomic() by adding transaction handling inside
the 'for' loop (and applying the changes from the early part of the
patch which go along with that).
builtin/receive-pack.c | 69 --
1 file changed, 50 insertions(+), 19 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 06eb287..5f44466 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -67,6 +67,7 @@ static const char *NONCE_SLOP = SLOP;
static const char *nonce_status;
static long nonce_stamp_slop;
static unsigned long nonce_stamp_slop_limit;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -822,6 +823,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
}
if (is_null_sha1(new_sha1)) {
+ struct strbuf err = STRBUF_INIT;
if (!parse_object(old_sha1)) {
old_sha1 = NULL;
if (ref_exists(name)) {
@@ -831,35 +833,36 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
cmd-did_not_exist = 1;
}
}
- if (delete_ref(namespaced_name, old_sha1, 0)) {
- rp_error(failed to delete %s, name);
+ if (ref_transaction_delete(transaction,
+ namespaced_name,
+ old_sha1,
+ 0, old_sha1 != NULL,
+ push, err)) {
+ rp_error(%s, err.buf);
+ strbuf_release(err);
return failed to delete;
}
+ strbuf_release(err);
return NULL; /* good */
}
else {
struct strbuf err = STRBUF_INIT;
- struct ref_transaction *transaction;
-
if (shallow_update si-shallow_ref[cmd-index]
update_shallow_ref(cmd, si))
return shallow error;
- transaction = ref_transaction_begin(err);
- if (!transaction ||
- ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, push,
- err) ||
- ref_transaction_commit(transaction, err)) {
- ref_transaction_free(transaction);
-
+ if (ref_transaction_update(transaction,
+ namespaced_name,
+ new_sha1, old_sha1,
+ 0, 1, push,
+ err)) {
rp_error(%s, err.buf);
strbuf_release(err);
+
return failed to update ref;
}
-
- ref_transaction_free(transaction);
strbuf_release(err);
+
return NULL; /* good */
}
}
@@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,
the reported refs above);
}
+static void execute_commands_loop(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ struct strbuf err = STRBUF_INIT;
+
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ transaction = ref_transaction_begin(err);
+ if (!transaction) {
+ rp_error(%s, err.buf);
+
[PATCHv8 8/9] push.c: add an --atomic argument
From: Ronnie Sahlberg sahlb...@google.com
Add a command line argument to the git push command to request atomic
pushes.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v8:
no changes
v7:
Use OPT_BOOL instead of OPT_BIT.
This allows for --no-atomic option on the command line.
v6:
- OPT_BIT(0, atomic, flags, N_(use an atomic transaction
remote),
+ OPT_BIT(0, atomic, flags, N_(request atomic transaction on
remote side),
skipped v4 v5
v2-v3:
* s/atomic-push/atomic/
* s/the an/an/
* no serverside, but just remote instead
* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH
Changes v1 - v2
It's --atomic now! (dropping the -push)
Documentation/git-push.txt | 7 ++-
builtin/push.c | 5 +
transport.c| 1 +
transport.h| 1 +
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21b3f29..4764fcf 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
SYNOPSIS
[verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run]
[--receive-pack=git-receive-pack]
+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n |
--dry-run] [--receive-pack=git-receive-pack]
[--repo=repository] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream] [--signed]
[--force-with-lease[=refname[:expect]]]
@@ -136,6 +136,11 @@ already exists on the remote side.
logged. See linkgit:git-receive-pack[1] for the details
on the receiving end.
+--[no-]atomic::
+ Use an atomic transaction on the remote side if available.
+ Either all refs are updated, or on error, no refs are updated.
+ If the server does not support atomic pushes the push will fail.
+
--receive-pack=git-receive-pack::
--exec=git-receive-pack::
Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..8f1d945 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -487,6 +487,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
int flags = 0;
int tags = 0;
int rc;
+ int atomic = 0;
const char *repo = NULL;/* default repository */
struct option options[] = {
OPT__VERBOSITY(verbosity),
@@ -518,6 +519,7 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
OPT_BIT(0, follow-tags, flags, N_(push missing but relevant
tags),
TRANSPORT_PUSH_FOLLOW_TAGS),
OPT_BIT(0, signed, flags, N_(GPG sign the push),
TRANSPORT_PUSH_CERT),
+ OPT_BOOL(0, atomic, atomic, N_(request atomic transaction
on remote side)),
OPT_END()
};
@@ -533,6 +535,9 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
if (tags)
add_refspec(refs/tags/*);
+ if (atomic)
+ flags |= TRANSPORT_PUSH_ATOMIC;
+
if (argc 0) {
repo = argv[0];
set_refspecs(argv + 1, argc - 1, repo);
diff --git a/transport.c b/transport.c
index c67feee..1373152 100644
--- a/transport.c
+++ b/transport.c
@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport,
struct ref *remote_re
args.dry_run = !!(flags TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags TRANSPORT_PUSH_PORCELAIN);
args.push_cert = !!(flags TRANSPORT_PUSH_CERT);
+ args.atomic = !!(flags TRANSPORT_PUSH_ATOMIC);
args.url = transport-url;
ret = send_pack(args, data-fd, data-conn, remote_refs,
diff --git a/transport.h b/transport.h
index 3e0091e..18d2cf8 100644
--- a/transport.h
+++ b/transport.h
@@ -125,6 +125,7 @@ struct transport {
#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
#define TRANSPORT_PUSH_CERT 2048
+#define TRANSPORT_PUSH_ATOMIC 4096
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) -
gettext_width(x)), (x)
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 7/9] receive-pack.c: enable atomic push protocol support
This enables the atomic protocol option as implemented in the previous patches. Signed-off-by: Stefan Beller sbel...@google.com --- Notes: v8: no changes v7: * new with v7 of the patch series. * this was part of the first patch in the series, moved back here for bisectability builtin/receive-pack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 35a2264..61eba4e 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -173,7 +173,8 @@ static void show_ref(const char *path, const unsigned char *sha1) struct strbuf cap = STRBUF_INIT; strbuf_addstr(cap, - report-status delete-refs side-band-64k quiet); + report-status delete-refs side-band-64k quiet + atomic); if (prefer_ofs_delta) strbuf_addstr(cap, ofs-delta); if (push_cert_nonce) -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update
This renames ref_update_to_be_sent to check_to_send_update and inverts
the meaning of the return value. Having the return value inverted we
can have different values for the error codes. This is useful in a
later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED
case.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
This was introduced with the [PATCHv2] series.
Changes v2 - v3:
* Rename to check_to_send_update
* Negative error values.
* errors values are #define'd and not raw numbers.
skipped v4 v5
v6:
* negative #define'd values
v7, v8:
* no changes
send-pack.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 6d0c159..b7d8e01 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf
*sb)
for_each_commit_graft(advertise_shallow_grafts_cb, sb);
}
-static int ref_update_to_be_sent(const struct ref *ref, const struct
send_pack_args *args)
+#define CHECK_REF_NO_PUSH -1
+#define CHECK_REF_STATUS_REJECTED -2
+#define CHECK_REF_UPTODATE -3
+static int check_to_send_update(const struct ref *ref, const struct
send_pack_args *args)
{
if (!ref-peer_ref !args-send_mirror)
- return 0;
+ return CHECK_REF_NO_PUSH;
/* Check for statuses set by set_ref_status_for_push() */
switch (ref-status) {
@@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref,
const struct send_pack_a
case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_REJECT_STALE:
case REF_STATUS_REJECT_NODELETE:
+ return CHECK_REF_STATUS_REJECTED;
case REF_STATUS_UPTODATE:
- return 0;
+ return CHECK_REF_UPTODATE;
default:
- return 1;
+ return 0;
}
}
@@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,
strbuf_addstr(cert, \n);
for (ref = remote_refs; ref; ref = ref-next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
update_seen = 1;
strbuf_addf(cert, %s %s %s\n,
@@ -369,7 +373,7 @@ int send_pack(struct send_pack_args *args,
* the pack data.
*/
for (ref = remote_refs; ref; ref = ref-next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
if (!ref-deletion)
@@ -390,7 +394,7 @@ int send_pack(struct send_pack_args *args,
if (args-dry_run || args-push_cert)
continue;
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) 0)
continue;
old_hex = sha1_to_hex(ref-old_sha1);
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function
Update receive-pack to use an atomic transaction iff the client negotiated
that it wanted atomic push. This leaves the default behavior to be the old
non-atomic one ref at a time update. This is to cause as little disruption
as possible to existing clients. It is unknown if there are client scripts
that depend on the old non-atomic behavior so we make it opt-in for now.
If it turns out over time that there are no client scripts that depend on the
old behavior we can change git to default to use atomic pushes and instead
offer an opt-out argument for people that do not want atomic pushes.
Inspired-by: Ronnie Sahlberg sahlb...@google.com
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Changes in v8:
removed superflous } to make it compile again
Changes in v7:
Eric suggested to replace [PATCH 4/7] receive-pack.c:
receive-pack.c: use a single ref_transaction for atomic pushes
by smaller patches
This is the last patch replacing said large commit.
Changes in v6:
This is a complete rewrite of the patch essentially.
Eric suggested to split up the flow into functions, so
it is easier to read. It's more lines of code, but indeed
it's easier to follow. Thanks Eric!
Note there is another patch following this one
moving the helper functions above execute_commands.
I just choose the order of the functions in this patch
to have a better diff (just inserting the call to the function
execute_commands_non_atomic and that function directly follows.)
The next patch of the series will move that up.
Because of the rewrite and the fixes of the previous five
versions there is not much left of Ronnies original patch,
so I'll claim authorship of this one.
Changes v1 - v2:
* update(...) assumes to be always in a transaction
* Caring about when to begin/commit transactions is put
into execute_commands
v2-v3:
* meditated about the error flow. Now we always construct a local
strbuf err if required. Then the flow is easier to follow and
destruction of it is performed nearby.
* early return in execute_commands if transaction_begin fails.
v3-v4:
* revamp logic again. This should keep the non atomic behavior
as is (in case of error say so, in non error case just free the
transaction). In the atomic case we either do nothing (when no error),
or abort with the goto.
if (!cmd-error_string) {
if (!use_atomic
ref_transaction_commit(transaction, err)) {
ref_transaction_free(transaction);
rp_error(%s, err.buf);
strbuf_release(err);
cmd-error_string = failed to update ref;
}
} else if (use_atomic) {
goto atomic_failure;
} else {
ref_transaction_free(transaction);
}
* Having the goto directly there when checking for cmd-error_string,
we don't need to do it again, so the paragraph explaining the error
checking is gone as well. (Previous patch had the following, this is
put at the end of the function, where the goto jumps to and the
comment
has been dropped.
+ /*
+* update(...) may abort early (i.e. because the hook refused to
+* update that ref) which then doesn't even record a transaction
+* regarding that ref. Make sure all commands are without error
+* and then commit atomically.
+*/
+ for (cmd = commands; cmd; cmd = cmd-next)
+ if (cmd-error_string)
+ break;
v4-v5:
Eric wrote:
Repeating from my earlier review[1]: If the 'pre-receive' hook
declines, then this transaction is left dangling (and its resources
leaked).
You're right. The initialization of the transaction is now
near the actual loop after the pre receive hook.
The !use_atomic case (below), calls this error failed to start
transaction, not merely transaction error.
ok, now both are transaction failed to start.
In all cases where these generic errors are reported,
we do have a rp_error(...) with details.
Furthermore, in the use_atomic case (also below), when a commit fails,
you assign err.buf to cmd-error_string rather than a generic
transaction error message. What differs between these cases which
makes the generic message preferable here over the more specific
err.buf message?
They are the
[PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes
This adds tests for the atomic push option.
The first four tests check if the atomic option works in
good conditions and the last three patches check if the atomic
option prevents any change to be pushed if just one ref cannot
be updated.
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v7, v8: no changes
v6:
the same as v2, so I can resend the whole series as v6
v3 v4 v5 were skipped
Changes v1 - v2:
Please drop unused comments; they are distracting.
ok
It is not wrong per-se, but haven't you already tested in
combination with --mirror in the previous test?
I fixed the previous tests, so that there is no --mirror
and --atomic together. There is still a first --mirror push
for setup and a second with --atomic branchnames though
check_branches upstream master HEAD@{2} second HEAD~
A similar function test_ref_upstream is introduced.
What's the value of this test? Isn't it a non-fast-forward check
you already tested in the previous one?
I messed up there. Originally I wanted to test the 2 different
stages of rejection. A non-fast-forward check is done locally and
we don't even try pushing. But I also want to test if we locally
thing all is good, but the server refuses a ref to update.
This is now done with the last test named 'atomic push obeys
update hook preventing a branch to be pushed'. And that still fails.
I'll investigate that, while still sending out the series for another
review though.
* Redone the test helper, there is test_ref_upstream now.
This tests explicitely for SHA1 values of the ref.
(It's needed in the last test for example. The git push fails,
but still modifies the ref :/ )
* checked all chains and repaired them
* sometimes make use of git -C workdir
Notes v1:
Originally Ronnie had a similar patch prepared. But as I added
more tests and cleaned up the existing tests (e.g. use test_commit
instead of echo one file gitadd file git commit -a -m 'one',
removal of dead code), the file has changed so much that I'd rather
take ownership.
t/t5543-atomic-push.sh | 178 +
1 file changed, 178 insertions(+)
create mode 100755 t/t5543-atomic-push.sh
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
new file mode 100755
index 000..b81a542
--- /dev/null
+++ b/t/t5543-atomic-push.sh
@@ -0,0 +1,178 @@
+#!/bin/sh
+
+test_description='pushing to a repository using the atomic push option'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf workbench upstream
+ test_create_repo upstream
+ test_create_repo workbench
+ (
+ cd upstream
+ git config receive.denyCurrentBranch warn
+ )
+ (
+ cd workbench
+ git remote add up ../upstream
+ )
+}
+
+# Compare the ref ($1) in upstream with a ref value from workbench ($2)
+# i.e. test_refs second HEAD@{2}
+test_refs () {
+ test $# = 2
+ git -C upstream rev-parse --verify $1 expect
+ git -C workbench rev-parse --verify $2 actual
+ test_cmp expect actual
+}
+
+test_expect_success 'atomic push works for a single branch' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git push --mirror up
+ test_commit two
+ git push --atomic up master
+ )
+ test_refs master master
+'
+
+test_expect_success 'atomic push works for two branches' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git branch second
+ git push --mirror up
+ test_commit two
+ git checkout second
+ test_commit three
+ git push --atomic up master second
+ )
+ test_refs master master
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --mirror' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git checkout -b second
+ test_commit two
+ git push --atomic --mirror up
+ )
+ test_refs master master
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --force' '
+ mk_repo_pair
+ (
+ cd workbench
+ test_commit one
+ git branch second master
+ test_commit two_a
+ git checkout second
+ test_commit two_b
+ test_commit three_b
+ test_commit four
+ git push --mirror up
+ # The actual test is below
+ git checkout master
+ test_commit three_a
+ git checkout
[PATCHv8 3/9] send-pack.c: add --atomic command line argument
From: Ronnie Sahlberg sahlb...@google.com
This adds support to send-pack to negotiate and use atomic pushes
iff the server supports it. Atomic pushes are activated by a new command
line flag --atomic.
In order to do this we also need to change the semantics for send_pack()
slightly. The existing send_pack() function actually doesn't send all the
refs back to the server when multiple refs are involved, for example
when using --all. Several of the failure modes for pushes can already be
detected locally in the send_pack client based on the information from the
initial server side list of all the refs as generated by receive-pack.
Any such refs that we thus know would fail to push are thus pruned from
the list of refs we send to the server to update.
For atomic pushes, we have to deal thus with both failures that are detected
locally as well as failures that are reported back from the server. In order
to do so we treat all local failures as push failures too.
We introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can
flag all refs that we would normally have tried to push to the server
but we did not due to local failures. This is to improve the error message
back to the end user to flag that these refs failed to update since the
atomic push operation failed.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
Notes:
Changes v1 - v2:
* Now we only need a very small change in the existing code and have
a new static function, which cares about error reporting.
Junio wrote:
Hmph. Is atomic push so special that it deserves a separate
parameter? When we come up with yet another mode of failure,
would
we add another parameter to the callers to this function?
* error messages are worded differently (lower case!),
* use of error function instead of fprintf
* undashed the printed error message (atomic push failed);
Changes v2 - v3:
We avoid assignment inside a conditional.
Ok I switched to using a switch statement.
skipped v4 v5
v6:
* realign to one error string:
+ error(atomic push failed for ref %s. status: %d\n,
+ failing_ref-name, failing_ref-status);
* Use correct value now (negative defined from previous patch)
v7:
* return error(...); instead of error(...); return -1;
v8:
no changes
Documentation/git-send-pack.txt | 7 ++-
builtin/send-pack.c | 6 +-
remote.h| 3 ++-
send-pack.c | 39 +--
transport.c | 4
5 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..45c7725 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another
repository
SYNOPSIS
[verse]
-'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]
+'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic]
[host:]directory [ref...]
DESCRIPTION
---
@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush
packet.
Send a thin pack, which records objects in deltified form based
on objects not included in the pack to reduce network traffic.
+--atomic::
+ Use an atomic transaction for updating the refs. If any of the refs
+ fails to update then the entire push will fail without changing any
+ refs.
+
host::
A remote host to house the repository. When this
part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..b961e5a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
#include sha1-array.h
static const char send_pack_usage[] =
-git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [host:]directory
[ref...]\n
+git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=git-receive-pack] [--verbose] [--thin] [--atomic]
[host:]directory [ref...]\n
--all and explicit ref specification are mutually exclusive.;
static struct send_pack_args args;
@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char
*prefix)
args.use_thin_pack = 1;
continue;
}
+ if (!strcmp(arg, --atomic)) {
+ args.atomic = 1;
+ continue;
+ }
if
[PATCHv8 0/9] atomic pushes
The patch [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes was dropped and redone as 3 separate patches. This wasn't just done for doing it, but the end result has also changed. We have more smaller functions doing one thing instead of these larger functions. Thanks for the ideas, Eric! Also the advertisement of the atomic capabilites was moved to a later new patch in this series. This helps when you want to bisect this series later. Thanks Michael for pointing this out! Thanks, Stefan Ronnie Sahlberg (3): receive-pack.c: add documentation for atomic push support send-pack.c: add --atomic command line argument push.c: add an --atomic argument Stefan Beller (6): send-pack: rename ref_update_to_be_sent to check_to_send_update receive-pack.c: simplify execute_commands receive-pack.c: move transaction handling in a central place receive-pack.c: add execute_commands_atomic function receive-pack.c: enable atomic push protocol support t5543-atomic-push.sh: add basic tests for atomic pushes Documentation/git-push.txt| 7 +- Documentation/git-send-pack.txt | 7 +- Documentation/technical/protocol-capabilities.txt | 13 +- builtin/push.c| 5 + builtin/receive-pack.c| 168 +++- builtin/send-pack.c | 6 +- remote.h | 3 +- send-pack.c | 65 +++- send-pack.h | 3 +- t/t5543-atomic-push.sh| 178 ++ transport.c | 5 + transport.h | 1 + 12 files changed, 410 insertions(+), 51 deletions(-) create mode 100755 t/t5543-atomic-push.sh -- 2.2.1.62.g3f15098 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8 4/9] receive-pack.c: simplify execute_commands
No functional changes intended.
This commit shortens execute_commands by moving some parts of the code
to extra functions.
Suggested-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
Notes:
v8: no change
v7:
new in v7 as in v7 I'd split up the previous
[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction
for atomic pushes
as suggested by Eric.
This is pretty much
patch 1: Factor out code into helper functions which will be needed by
the upcoming atomic and non-atomic worker functions. Example helpers:
'cmd-error_string' and cmd-skip_update' check; and the
'si-shallow_ref[cmd-index]' check and handling.
builtin/receive-pack.c | 49 -
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4e8eaf7..06eb287 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command
*commands)
}
}
+static int should_process_cmd(struct command *cmd)
+{
+ if (cmd-error_string)
+ return 0;
+ if (cmd-skip_update)
+ return 0;
+ return 1;
+}
+
+static void check_shallow_bugs(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ if (shallow_update si-shallow_ref[cmd-index]) {
+ error(BUG: connectivity check has not been run on ref
%s,
+ cmd-ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (shallow_update !checked_connectivity)
+ error(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd-next) {
- if (cmd-error_string)
- continue;
-
- if (cmd-skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd-error_string = update(cmd, si);
- if (shallow_update !cmd-error_string
- si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on ref
%s,
- cmd-ref_name);
- checked_connectivity = 0;
- }
}
-
- if (shallow_update !checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ check_shallow_bugs(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
saving git push --signed certificate blobs
Hello, Just wanted to say there's a little script at [1] that saves the certificate blobs generated on the server side by git push --signed. Quoting from the source: # Collects the cert blob on push and saves it, then, if a certain number of # signed pushes have been seen, processes all the saved blobs in one go, # adding them to the special ref 'refs/push-certs'. This is done in a way # that allows searching for all the certs pertaining to one specific branch # (thanks to Junio Hamano for this idea plus general brainstorming). Note that although I posted it in the gitolite ML, this has very little to do with gitolite. Any git server can use it, with only one very minor change [2] needed. sitaram [1]: https://groups.google.com/forum/#!topic/gitolite/7cSrU6JorEY [2]: Either set the GL_OPTIONS_GPC_PENDING environment variable by reading its value from 'git config', or replace the only line that uses that variable, with some other test. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller sbel...@google.com wrote:
No functional changes intended.
This is useful to know but is of secondary importance, thus should be
placed after the explanation and justification of the change.
Subject: receive-pack.c: simplify execute_commands
This commit shortens execute_commands by moving some parts of the code
to extra functions.
The _real_ reason for moving these bits of code into their own
functions is that you intend to introduce additional callers in
subsequent patches. That is what the commit message (including
subject) should be conveying to the reader.
The claimed simplification is questionable and not of particular
importance; and it's misleading to paint it as a goal of the patch.
Consequently, you could drop mention of it altogether.
More below.
Suggested-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4e8eaf7..06eb287 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command
*commands)
}
}
+static int should_process_cmd(struct command *cmd)
+{
+ if (cmd-error_string)
+ return 0;
+ if (cmd-skip_update)
+ return 0;
+ return 1;
Alternately, depending upon the polarity of your brain, you could
collapse the entire function body to:
return !cmd-error_string !cmd-skip_update;
or:
return !(cmd-error_string || cmd-skip_update);
+}
+
+static void check_shallow_bugs(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ if (shallow_update si-shallow_ref[cmd-index]) {
Here, inside the loop, you check 'shallow_update'...
+ error(BUG: connectivity check has not been run on
ref %s,
+ cmd-ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (shallow_update !checked_connectivity)
And here, after the loop, you check 'shallow_update'.
But, if you examine the overall logic, you will find that this
function does _nothing_ at all when 'shallow_update' is false (other
than uselessly looping through 'commands'). Therefore, either check
'shallow_update' just once at the beginning of the function and exit
early if false, or have the caller check 'shallow_update' and only
invoke check_shallow_bugs() if true.
Also, since nothing happens between them, the two conditionals inside
the loop can be coalesced:
if (should_process_cmd(cmd) si-shallow_ref[cmd-index]) {
+ error(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
In v6, you considered this a fatal error in the atomic case, which
caused the entire transaction to be rolled back. However, in this
version, this error has no effect whatsoever on the atomic
transaction, which is a rather significant behavioral departure. Which
is correct? (This is a genuine question; not at all rhetorical.) If
failing the entire transaction is the correct thing to do, then this
is going to need some more work.
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd-next) {
- if (cmd-error_string)
- continue;
-
- if (cmd-skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd-error_string = update(cmd, si);
- if (shallow_update !cmd-error_string
- si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on
ref %s,
- cmd-ref_name);
- checked_connectivity = 0;
- }
}
-
- if (shallow_update !checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ check_shallow_bugs(commands, si);
}
static struct command **queue_command(struct command **tail,
Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller sbel...@google.com wrote:
Subject: receive-pack.c: add documentation for atomic push support
This patch is doing a lot more than merely adding documentation. It's
also updating send-pack and receive-pack to be able to negotiate the
new protocol capability atomic. The fact that you removed the actual
advertisement of atomic from this patch doesn't turn it into a
documentation-only patch.
When Michael raised the issue of the server being broken due to
advertising a capability which it does not yet implement, his
recommendation[1] was to reorder the patches, not to split out the one
tiny bit (capability advertisement) from the larger change. Was there
an insurmountable conflict which prevented you from reordering the
patches? This is a genuine question since splitting off advertisement
-- and only advertisement -- to a patch later in the series smells
like a least-resistance approach, rather than a proper solution to the
issue Michael raised.
[1]: http://article.gmane.org/gmane.comp.version-control.git/261793
This documents the protocol option between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it. The capability is also not yet advertised
by receive-pack as git doesn't know how to handle it yet.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..4f8a7bf 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client
requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress
server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic
+--
+
+If the server sends the 'atomic' capability it is capable of accepting
+atomic pushes. If the pushing client requests this capability, the server
+will update the refs in one atomic transaction. Either all refs are
+updated or none.
+
allow-tip-sha1-in-want
--
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..4e8eaf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -1179,6 +1180,8 @@ static struct command *read_head_info(struct sha1_array
*shallow)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, quiet))
quiet = 1;
+ if (parse_feature_request(feature_list, atomic))
+ use_atomic = 1;
}
if (!strcmp(line, push-cert)) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..6d0c159 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int use_atomic = 0;
+ int atomic_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,
agent_supported = 1;
if (server_supports(no-thin))
args-use_thin_pack = 0;
+ if (server_supports(atomic))
+ atomic_supported = 1;
if (args-push_cert) {
int len;
@@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
Perhaps you should specify a branch such as
'master'.\n);
return 0;
}
+ if (args-atomic !atomic_supported)
+
Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller sbel...@google.com wrote:
No functional changes intended.
This commit shortens execute_commands by moving some parts of the code
to extra functions.
Suggested-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4e8eaf7..06eb287 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command
*commands)
}
}
+static void check_shallow_bugs(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+ for (cmd = commands; cmd; cmd = cmd-next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ if (shallow_update si-shallow_ref[cmd-index]) {
Another issue: In the original code, 'si-shallow_ref[cmd-index]' is
only checked if cmd-error_string is NULL, but here you check it
unconditionally, despite the commit message claiming no functional
changes. Did you verify that such a behavioral change is benign? (This
is a genuine question.)
+ error(BUG: connectivity check has not been run on
ref %s,
+ cmd-ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (shallow_update !checked_connectivity)
+ error(BUG: run 'git fsck' for safety.\n
+ If there are errors, try to remove
+ the reported refs above);
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup(HEAD, 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd-next) {
- if (cmd-error_string)
- continue;
-
- if (cmd-skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd-error_string = update(cmd, si);
- if (shallow_update !cmd-error_string
- si-shallow_ref[cmd-index]) {
- error(BUG: connectivity check has not been run on
ref %s,
- cmd-ref_name);
- checked_connectivity = 0;
- }
}
-
- if (shallow_update !checked_connectivity)
- error(BUG: run 'git fsck' for safety.\n
- If there are errors, try to remove
- the reported refs above);
+ check_shallow_bugs(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
