Jeff King [off-list ref] writes:
On Wed, Nov 28, 2007 at 02:14:03PM -0800, Steven Grimm wrote:
quoted
@@ -177,7 +179,16 @@ static int receive_status(int in, struct ref *refs)
line[strlen(line)-1] = '\0';
refname = line + 3;
- msg = strchr(refname, ' ');
+ newsha1_hex = strchr(refname, ' ');
+ if (newsha1_hex) {
+ *newsha1_hex++ = '\0';
+ if (get_sha1_hex(newsha1_hex, newsha1)) {
+ fprintf(stderr, "protocol error: bad sha1 %s\n",
+ newsha1_hex);
+ newsha1_hex = NULL;
+ }
+ }
+ msg = strchr(newsha1_hex, ' ');
if (msg)
*msg++ = '\0';
Doesn't this always put the first "word" of a response into newsha1_hex?
We want to do this only for 'ok' responses; 'ng' responses are already
using that space as part of the error message.
I do not think reporting back the rewritten object name makes much sense
nor adds any value; it won't be useful information until you fetch the
object.
I do not think reporting back _anything_ other than "ok" adds much value
at all. Sure, if the update hook did something funky you would get such
a report, but the situation is not any different if some warm body is
sitting on the other end and building on top of what you pushed
immediately he sees any push into the repository, and in such a case
your git-push would not get any such reporting anyway.
We do not even have to worry about this reporting at all if we do not
allow munging the refs in the update hook. In a sense, this patch is
creating a problem that does not need to be solved. Perhaps modifying
update hook to allow so makes it possible to munge refs while holding a
lock, but is it really worth this hassle? Isn't there a better way, I
wonder?
On Nov 28, 2007, at 3:42 PM, Junio C Hamano wrote:
I do not think reporting back the rewritten object name makes much
sense
nor adds any value; it won't be useful information until you fetch the
object.
Right, this was mostly in anticipation of doing an automatic fetch, so
that I would avoid fetching anything but the rewritten revisions; if I
just fetched the remote ref as normal, then I'd potentially pick up
unrelated changes that happened to hit just after my pack was
accepted, which wouldn't maintain the "update the tracking ref to
point to what I just pushed" semantics.
Since it sounds like that's a nonstarter, I agree this part of the
patch isn't useful.
I do not think reporting back _anything_ other than "ok" adds much
value
at all. Sure, if the update hook did something funky you would get
such
a report, but the situation is not any different if some warm body is
sitting on the other end and building on top of what you pushed
immediately he sees any push into the repository, and in such a case
your git-push would not get any such reporting anyway.
I disagree that it's the same. In this case the updated ref happens as
a component of the push operation (which of course includes running
update hooks and at the very least looking at their exit codes to see
if a change should be rejected), not as a result of some other process
that happens to occur at nearly the same time. Reporting back the new
ref, at the very least, tells you that it's not useful to update the
tracking ref since it's 100% guaranteed to be wrong by the time the
push finishes.
We do not even have to worry about this reporting at all if we do not
allow munging the refs in the update hook. In a sense, this patch is
creating a problem that does not need to be solved. Perhaps modifying
update hook to allow so makes it possible to munge refs while
holding a
lock, but is it really worth this hassle? Isn't there a better way, I
wonder?
If there is, I'm happy to hear it; for me this patch is a means, not
an end. What I actually want is to be able to have a particular set of
branches in a particular git repository be as-transparent-as-possible
conduits to corresponding branches in an svn repository.
I arrived at this approach by following this train of thought:
1. The update hook is the only hook that allows me to reject the push,
which I need to do if svn refuses to accept the change.
2. To tell whether svn accepts a change, I need to run git-svn
dcommit; thanks to #1, I need to do that from inside the update hook.
3. When I commit, git-svn needs to track that the git revision now
corresponds to an svn revision. It does that by modifying the commit
message to add its git-svn-id: line.
4. Modifying the commit comment causes the revision's SHA1 to change.
5. Out-of-the-box push thinks a push has failed if the ref's SHA1
changes in the update hook.
6. Therefore push needs to be modified to not do that.
If any one of #1-5 wasn't true or was solvable in a different way,
then #6 wouldn't be needed. For example, if git-svn kept its mapping
of git revisions to svn revisions somewhere else it could leave the
commit messages untouched, meaning the SHA1s wouldn't change.
-Steve