[PATCH] Add extra logic required to detect endianness on Solaris
Signed-off-by: Charles Bailey cbaile...@bloomberg.net --- The endian detection added in 7e3dae494 isn't sufficient for the Solaris Studio compilers. This adds some fallback logic which works for Solaris but would also work for AIX and Linux if it were needed. compat/bswap.h | 21 + 1 file changed, 21 insertions(+) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..5a41311 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -110,6 +110,27 @@ static inline uint64_t git_bswap64(uint64_t x) #endif #if !defined(__BYTE_ORDER) +/* Known to be needed on Solaris but designed to potentially more portable */ + +#if !defined(__BIG_ENDIAN) +#define __BIG_ENDIAN 4321 +#endif + +#if !defined(__LITTLE_ENDIAN) +#define __LITTLE_ENDIAN 1234 +#endif + +#if defined(_BIG_ENDIAN) +#define __BYTE_ORDER __BIG_ENDIAN +#endif + +#if defined(_LITTLE_ENDIAN) +#define __BYTE_ORDER __LITTLE_ENDIAN +#endif + +#endif /* !defined(__BYTE_ORDER) */ + +#if !defined(__BYTE_ORDER) # error Cannot determine endianness #endif -- 1.9.0 -- 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: Pull is Evil
On Wed, Apr 30, 2014 at 05:25:59PM -0500, Felipe Contreras wrote: Marc Branchaud wrote: On 14-04-30 04:14 PM, Felipe Contreras wrote: What is wrong when `git pull` merges a fast-forward? Nothing. Everything. It depends. It depends on what? I don't see how a fast-forward `git pull` could possibly have any trouble. The problems with `git pull` come when you can't do a fast-forward merge, right? Some of them, maybe most of them. Name one problem with a fast-forward merge. At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD So there are times when fast-forward merges are the right thing, and times when they're not, and as you can see, this depends on context and isn't per-repository. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: Pull is Evil
brian m. carlson wrote: On Wed, Apr 30, 2014 at 05:25:59PM -0500, Felipe Contreras wrote: Marc Branchaud wrote: On 14-04-30 04:14 PM, Felipe Contreras wrote: What is wrong when `git pull` merges a fast-forward? Nothing. Everything. It depends. It depends on what? I don't see how a fast-forward `git pull` could possibly have any trouble. The problems with `git pull` come when you can't do a fast-forward merge, right? Some of them, maybe most of them. Name one problem with a fast-forward merge. At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD So there are times when fast-forward merges are the right thing, and times when they're not, and as you can see, this depends on context and isn't per-repository. That's not what I asked. I didn't ask you if fast-forward merges were the right thing to do in every situation. I asked you, *when* people do a fast-forward merge (that is; when it's possible and desirable), what are the problems that a fast-forward merge causes? I tired of waiting, so I'll answer for you: there are absolutely no problems. The problems are only on non-fast-forward merges, and we have a solution. -- Felipe Contreras -- 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] Define constants for lengths of object names
Using preprocessor constants rather than hardcoded numbers is considered a
good programming practice. Provide two constants, GIT_OID_RAWSZ and
GIT_OID_HEXSZ, which are the lengths of an SHA-1 object name in bytes and
hex digits, respectively. These names are the same as those used by
libgit2.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
object.h | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/object.h b/object.h
index 6e12f2c..f1cff2d 100644
--- a/object.h
+++ b/object.h
@@ -1,6 +1,13 @@
#ifndef OBJECT_H
#define OBJECT_H
+/*
+ * The length in bytes and in hex digits of an object name (SHA-1 value).
+ * These are the same names used by libgit2.
+ */
+#define GIT_OID_RAWSZ 20
+#define GIT_OID_HEXSZ 40
+
struct object_list {
struct object *item;
struct object_list *next;
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
- unsigned char sha1[20];
+ unsigned char sha1[GIT_OID_RAWSZ];
};
extern const char *typename(unsigned int type);
--
2.0.0.rc0
--
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/8] CodingGuidelines: typofix
Junio C Hamano gits...@pobox.com writes: The sentence lacked the necessary verb. No, it didn't. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index f424dbd..fdf6269 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -15,7 +15,7 @@ code. For Git in general, three rough rules are: let's use it. Again, we live in the real world, and it is sometimes a ^^ - judgement call, the decision based more on real world + judgement call, the decision is based more on real world constraints people face than what the paper standard says. There is one common is for original statement and paraphrase. Adding another one turns this into two sentences which cannot sensibly be connected with a comma. If you want to fix something here, do s/judgement/judgment/ instead. -- David Kastrup -- 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/8] CodingGuidelines: call the conditional statement if (), not if()
Junio C Hamano gits...@pobox.com writes: The point immediately before it is about having SP after the control keyword. Follow it. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1b0cd2b..21e4272 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -184,7 +184,7 @@ For C programs: of else if statements, it can make sense to add braces to single line blocks. - - We try to avoid assignments inside if(). + - We try to avoid assignments inside if () condition. That's not grammatical. -- David Kastrup -- 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 4/8] CodingGuidelines: give an example for control statements
Junio C Hamano gits...@pobox.com writes: Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 11 +++ 1 file changed, 11 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1e0c4cf..d72e912 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -97,6 +97,17 @@ For shell scripts specifically (not exhaustive): then should be on the next line for if statements, and do should be on the next line for while and for. + (incorrect) + if test -f hello; then + do this + fi + + (correct) + if test -f hello + then + do this + fi + - We prefer test over [ ... ]. - We do not write the noiseword function in front of shell s/noiseword/bashism/ -- David Kastrup -- 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: Pull is Evil
On 14-05-01 05:46 AM, brian m. carlson wrote: On Wed, Apr 30, 2014 at 05:25:59PM -0500, Felipe Contreras wrote: Marc Branchaud wrote: On 14-04-30 04:14 PM, Felipe Contreras wrote: What is wrong when `git pull` merges a fast-forward? Nothing. Everything. It depends. It depends on what? I don't see how a fast-forward `git pull` could possibly have any trouble. The problems with `git pull` come when you can't do a fast-forward merge, right? Some of them, maybe most of them. Name one problem with a fast-forward merge. At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD Thanks for the nice example. To me this looks like an advanced use of git pull. A new user could be taught to work like this, but I don't think a new user would come up with it on their own (until they became an experienced user). What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD I suggest that this approach is superior for new users (despite the increased risk of finger cramps), because if main-repo's maintenance-branch is updated in the interim and the push fails, the user can use the exact same commands to resolve the situation. Sure, the non-pull approach makes use of Scary Branch Stuff (remotes and namespaces and detached HEADs -- oh my!). But trying to avoid that stuff is precisely the slippery slope that led to pull's misguided gymnastics. We've gone down that slope, slipped and fallen over, and now we're wallowing in the muck. M. -- 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: Pull is Evil
Felipe Contreras felipe.contre...@gmail.com writes: brian m. carlson wrote: .. At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD So there are times when fast-forward merges are the right thing, and times when they're not, and as you can see, this depends on context and isn't per-repository. That's not what I asked. I didn't ask you if fast-forward merges were the right thing to do in every situation. I asked you, *when* people do a fast-forward merge (that is; when it's possible and desirable), what are the problems that a fast-forward merge causes? But then I think you asked a wrong question. The opposite case of the question tells me what is wrong in it: When people do a real merge (that is: when it's possible and desirable), there is no reason to forbid 'git pull' from creating a real merge. What are the problems that a real merge causes under that condition? By definition, because of when it's possible and DESIRABLE part, the answer is absolutely zero. That is not an interesting question, is it? My reading of the design of the let's forbid non-ff merge when people do 'git pull' is based on this reasonong: - Most people are not integrators, and letting git pull run on their work based on a stale upstream to sync with an updated upstream would create a merge in a wrong direction and letting user continue on it. We need to have a way to prevent this. - Forbid git pull when the HEAD is based on a stale upstream, i.e. the pull does not fast-forward. Integrators that would want to _allow_ real merges may be inconvenienced so we will give a configuration to let them say that with pull.mode=merge. - We do not forbid git pull if the pull will fast-forward. We do not do anything for that case, because everybody will accept fast-forward, whether he is a contributor or an integrator. Doesn't Brian's case show the justification because everybody will accept fast-forward does not hold? It shows that the user do not necessarily know when it's possible and DESIRABLE, and updating the command is about helping people avoid an action that may not be desirable in the end. Brian needs a way to make sure he fast-forwards when pulling the project's maintenance-branch into his maintenance-branch, and also he does *not* fast-forward when pulling developer's fix branch into that same maintenance-branch of his. So neither pull.mode nor branch.*.pullmode would help him and the example may show we need a bit more work to help that case, no? -- 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] Define constants for lengths of object names
Hi,
brian m. carlson wrote:
--- a/object.h
+++ b/object.h
[...]
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
- unsigned char sha1[20];
+ unsigned char sha1[GIT_OID_RAWSZ];
Maybe my brain has been damaged by reading code from too many C
projects that hard-code some constants, but I find '20' easier to read
here.
What happened to the
struct object_id {
unsigned char id[20];
};
...
struct object {
...
struct object_id id;
};
idea?
Thanks,
Jonathan
--
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/8] CodingGuidelines: typofix
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: The sentence lacked the necessary verb. No, it didn't. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/CodingGuidelines | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index f424dbd..fdf6269 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -15,7 +15,7 @@ code. For Git in general, three rough rules are: let's use it. Again, we live in the real world, and it is sometimes a ^^ - judgement call, the decision based more on real world + judgement call, the decision is based more on real world constraints people face than what the paper standard says. There is one common is for original statement and paraphrase. Adding another one turns this into two sentences which cannot sensibly be connected with a comma. Thanks for spotting. I thought (but I see I didn't by mistake) that I split them into two sentences, replacing the comma with a semicolon. If you want to fix something here, do s/judgement/judgment/ instead. That too. -- 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] Re: Pull is Evil
On Thu, May 01, 2014 at 11:20:44AM -0400, Marc Branchaud wrote: On 14-05-01 05:46 AM, brian m. carlson wrote: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD … What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You're missing Brian's fast-forward merge here. It should be: git checkout maintenance-branch git fetch main-repo git merge --ff-only main-repo/maintenance-branch git fetch developer-remote … Sure, the non-pull approach makes use of Scary Branch Stuff (remotes and namespaces and detached HEADs -- oh my!). No need for detached heads with Brian's local maintenance-branch. If you're teaching and just need folks merging the remote's HEAD, you can avoid namespaces and remotes entirely: git fetch git://example.net/main-repo.git git merge --ff-only FETCH_HEAD although I doubt “the remote's HEAD” will be easier to explain than the namespaced, remote-tracking branches it replaces. It's certainly not worth the hassle of un-training FETCH_HEAD-merges later on ;). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 4/8] CodingGuidelines: give an example for control statements
David Kastrup d...@gnu.org writes: - We do not write the noiseword function in front of shell s/noiseword/bashism/ That is outside the scope of this patch, but since you brought it up... I did consider between noiseword and bashism when I wrote this part, and decided against bashism. XCU 2.4 Reserved Words lists it (among others) and says ... may be recognized as reserved words on some implementations ... causing unspecified results Even if bash were not the only shell that uses function keyword to introduce a shell function definition, we wouldn't use it. As we say in the introductory part, we may say It is not in POSIX, but it is supported so widely and using it give us so great a benefit, so we do use it for some things, but function is not one of them. The reason is because it is a noiseword and its use is not necessary in order to define a shell function. -- 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] Re: Pull is Evil
On 14-05-01 01:56 PM, W. Trevor King wrote: On Thu, May 01, 2014 at 11:20:44AM -0400, Marc Branchaud wrote: On 14-05-01 05:46 AM, brian m. carlson wrote: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD … What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You're missing Brian's fast-forward merge here. It should be: git checkout maintenance-branch git fetch main-repo git merge --ff-only main-repo/maintenance-branch git fetch developer-remote … I think you're mistaken -- I checked out main-repo/maintenance-branch directly, so there's no need to fast-forward a local branch. Sure, the non-pull approach makes use of Scary Branch Stuff (remotes and namespaces and detached HEADs -- oh my!). No need for detached heads with Brian's local maintenance-branch. Yes. OTOH, no need to bother keeping a local maintenance-branch up to date if you use a detached HEAD. If you're teaching and just need folks merging the remote's HEAD, you can avoid namespaces and remotes entirely: git fetch git://example.net/main-repo.git git merge --ff-only FETCH_HEAD although I doubt “the remote's HEAD” will be easier to explain than the namespaced, remote-tracking branches it replaces. It's certainly not worth the hassle of un-training FETCH_HEAD-merges later on ;). Agreed. I wouldn't advocate teaching people about FETCH_HEAD as if it were something they should use regularly. M. -- 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/8] CodingGuidelines: call the conditional statement if (), not if()
David Kastrup d...@gnu.org writes: - - We try to avoid assignments inside if(). + - We try to avoid assignments inside if () condition. That's not grammatical. OK, ... inside the condition part of an if () statement. then? -- 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] Define constants for lengths of object names
Jonathan Nieder wrote:
brian m. carlson wrote:
--- a/object.h
+++ b/object.h
[...]
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
-unsigned char sha1[20];
+unsigned char sha1[GIT_OID_RAWSZ];
Maybe my brain has been damaged by reading code from too many C
projects that hard-code some constants, but I find '20' easier to read
here.
That said, RAW_SHA1_BYTES would sound okay to me.
Part of the problem was how long it takes to mentally parse oid_rawsz.
Thanks,
Jonathan
--
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/8] CodingGuidelines: call the conditional statement if (), not if()
Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: - - We try to avoid assignments inside if(). + - We try to avoid assignments inside if () condition. That's not grammatical. OK, ... inside the condition part of an if () statement. then? ... in the condition of an if statement is what I would write. if () statement is mixing name and shorthand for the statement. -- David Kastrup -- 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 v10 11/12] Documentation: add documentation for 'git interpret-trailers'
From: Jeremy Morton ad...@game-point.net On 29/04/2014 12:47, Christian Couder wrote: Also, if there were no current branch name because you're committing in a detached head state, it would be nice if you could have some logic to determine that, and instead write the trailer as: Made-on-branch: (detached HEAD: AB12CD34) You may need to write a small script for that. Then you just need the trailer.m-o-b.command config value to point to your script. ... or whatever. And also how about some logic to be able to say that if you're committing to the master branch, the trailer doesn't get inserted at all? You can script that too. But it would be nicer if the logic were built-in, then you wouldn't have to share some script with your work colleagues. :-) The above logic is very specific to your workflow. For example some people might a Made-on-branch: trailer only when they are on real branches except dev and master. Best, 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
Re: [PATCH] Add extra logic required to detect endianness on Solaris
Charles Bailey cbaile...@bloomberg.net writes: #if !defined(__BYTE_ORDER) +/* Known to be needed on Solaris but designed to potentially more portable */ + +#if !defined(__BIG_ENDIAN) +#define __BIG_ENDIAN 4321 +#endif + +#if !defined(__LITTLE_ENDIAN) +#define __LITTLE_ENDIAN 1234 +#endif + +#if defined(_BIG_ENDIAN) +#define __BYTE_ORDER __BIG_ENDIAN +#endif +#if defined(_LITTLE_ENDIAN) +#define __BYTE_ORDER __LITTLE_ENDIAN +#endif The existing support is only for platforms where all three macros (BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN) are defined, and the convention used on such platforms where BYTE_ORDER is set to either one of the *_ENDIAN macros to tell the code which byte order we have. This mimics the convention where __BYTE_ORDER and other two macros are already defined with two leading underscores, and in such a case we do not have to do anything. We make the final decision to use or bypass bswap64() in our ntohll() implementation based on the variables with double leading underscores. This patch seems to address two unrelated issues in that. (1) The existing support does not help a platform where the convention is to define either _BIG_ENDIAN (with one leading underscore) or _LITTLE_ENDIAN and not both, which is Solaris but there may be others. (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already defined on the platform. Or these may not have been defined at all. You avoid unconditionally redefing these. I find the latter iffy. What is the reason for avoiding redefinition? Is it because you know the original values they have are precious? And if so in what way they are precious? If the reason of avoiding redefinition is because you do not even know what their values are (so that you are trying to be safe by preserving), what other things can you say about their values you are preserving? Specifically, do you know that these two are defined differently, so that defining __BYTE_ORDER to one of them and comparing it to __BIG_ENDIAN is a good way to tell if the platform is big endian? I would understand it if (2) were we undefine if these are defined and then define them as 4321 and 1234 respectively, in order to avoid a compiler warning against redefinition of a macro, but that is not what I am seeing, so I am not sure what you meant to achieve by that if !defined() constructs. 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] Add extra logic required to detect endianness on Solaris
Junio C Hamano gits...@pobox.com writes: Charles Bailey cbaile...@bloomberg.net writes: #if !defined(__BYTE_ORDER) +/* Known to be needed on Solaris but designed to potentially more portable */ + +#if !defined(__BIG_ENDIAN) +#define __BIG_ENDIAN 4321 +#endif + +#if !defined(__LITTLE_ENDIAN) +#define __LITTLE_ENDIAN 1234 +#endif + +#if defined(_BIG_ENDIAN) +#define __BYTE_ORDER __BIG_ENDIAN +#endif +#if defined(_LITTLE_ENDIAN) +#define __BYTE_ORDER __LITTLE_ENDIAN +#endif The existing support is only for platforms where all three macros (BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN) are defined, and the convention used on such platforms where BYTE_ORDER is set to either one of the *_ENDIAN macros to tell the code which byte order we have. This mimics the convention where __BYTE_ORDER and other two macros are already defined with two leading underscores, and in such a case we do not have to do anything. We make the final decision to use or bypass bswap64() in our ntohll() implementation based on the variables with double leading underscores. This patch seems to address two unrelated issues in that. (1) The existing support does not help a platform where the convention is to define either _BIG_ENDIAN (with one leading underscore) or _LITTLE_ENDIAN and not both, which is Solaris but there may be others. (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already defined on the platform. Or these may not have been defined at all. You avoid unconditionally redefing these. I find the latter iffy. What is the reason for avoiding redefinition? Is it because you know the original values they have are precious? And if so in what way they are precious? If the reason of avoiding redefinition is because you do not even know what their values are (so that you are trying to be safe by preserving), what other things can you say about their values you are preserving? Specifically, do you know that these two are defined differently, so that defining __BYTE_ORDER to one of them and comparing it to __BIG_ENDIAN is a good way to tell if the platform is big endian? I would understand it if (2) were we undefine if these are defined and then define them as 4321 and 1234 respectively, in order to avoid a compiler warning against redefinition of a macro, but that is not what I am seeing, so I am not sure what you meant to achieve by that if !defined() constructs. Thanks. Just a thought. I am wondering if you may want to go the other way around. That is, instead of using we have byte-order, big and little and the way to determine endianness is to see byte-order matches which of the latter two, use there may be either big or little but not both defined, and that is how you learn the byte-order. And make these two macros match what Solaris happens to use. I am not sure which variant I like better myself, though. compat/bswap.h | 21 + 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..e87998e 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -101,19 +101,24 @@ static inline uint64_t git_bswap64(uint64_t x) #undef ntohll #undef htonll -#if !defined(__BYTE_ORDER) -# if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN) -# define __BYTE_ORDER BYTE_ORDER -# define __LITTLE_ENDIAN LITTLE_ENDIAN -# define __BIG_ENDIAN BIG_ENDIAN -# endif +#if !defined(_BIG_ENDIAN) !defined(_LITTLE_ENDIAN) + +#if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN) +# if BYTE_ORDER == BIG_ENDIAN +# define _BIG_ENDIAN +# else +# define _LITTLE_ENDIAN +#endif + #endif -#if !defined(__BYTE_ORDER) +#if !defined(_BIG_ENDIAN) !defined(_LITTLE_ENDIAN) # error Cannot determine endianness +#elif defined(_BIG_ENDIAN) defined(_LITTLE_ENDIAN) +# error Your endianness is screwed up #endif -#if __BYTE_ORDER == __BIG_ENDIAN +#if defined (_BIG_ENDIAN) # define ntohll(n) (n) # define htonll(n) (n) #else -- 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] Add extra logic required to detect endianness on Solaris
Junio C Hamano gits...@pobox.com writes: Just a thought. I am wondering if you may want to go the other way around. That is, instead of using we have byte-order, big and little and the way to determine endianness is to see byte-order matches which of the latter two, use there may be either big or little but not both defined, and that is how you learn the byte-order. And make these two macros match what Solaris happens to use. I am not sure which variant I like better myself, though. ... The how-about-this patch in the pregvious message forgets the default case where byte-order, little and big are defined with leading double underscore, which must also be handled, if we want to take this aproach. -- 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: Pull is Evil
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: brian m. carlson wrote: .. At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD So there are times when fast-forward merges are the right thing, and times when they're not, and as you can see, this depends on context and isn't per-repository. That's not what I asked. I didn't ask you if fast-forward merges were the right thing to do in every situation. I asked you, *when* people do a fast-forward merge (that is; when it's possible and desirable), what are the problems that a fast-forward merge causes? But then I think you asked a wrong question. I asked the simple uncontroversial question as a rhetorical aid. I hoped I would get the obvious answer, but I didn't even get that, so what hope is there of convincing these people of the one that needs real pondering? The opposite case of the question tells me what is wrong in it: When people do a real merge (that is: when it's possible and desirable), there is no reason to forbid 'git pull' from creating a real merge. What are the problems that a real merge causes under that condition? By definition, because of when it's possible and DESIRABLE part, the answer is absolutely zero. That is not an interesting question, is it? That's right, but we are discussing the default behavior of `git pull`, which if we agree has no problems when the ff is a) possible, and b) desirable, the problems must come when either one of those is not met. a) If the fast-forward is not possible, that creates problems, because a real merge might happen, and it's not desirable. However, if we don't allow real merges to happen by default this couldn't be a problem. b) If the fast-forward is not desirable, then the user wouldn't be running `git pull`, would be running `git pull --no-ff`. In other words, after the proposed changes `git pull` by default would have no issues. Doesn't Brian's case show the justification because everybody will accept fast-forward does not hold? It shows that the user do not necessarily know when it's possible and DESIRABLE, and updating the command is about helping people avoid an action that may not be desirable in the end. No, it doesn't hold. As I said, if we change the default the fact that it's not possible is not an issue. The only problem would be when it's not desirable, however, that's a problem of the user's ignorance, and the failure of the project's policity to communicate clearly to him that he should be running `git merge --no-ff`. There's absolutely nothing we can do to help him. The only thing we could do is not allow fast-forward merges either, in which case `git pull` becomes a no-op that can't possibly do anything ever. Brian needs a way to make sure he fast-forwards when pulling the project's maintenance-branch into his maintenance-branch, and also he does *not* fast-forward when pulling developer's fix branch into that same maintenance-branch of his. First of all this use-case is not realistic. The moment he merges a developer branch, hes maintenance and the probject's diverge, and all the pulls after that cannot be fast-forward. It's pointless to add something that just doesn't happen. It will be possible to do the fast-forward merges only early on the life of this branch, not afterwards. For this short period of time he can just simply use his fingers to type `git merge --no-ff`. -- Felipe Contreras -- 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: Pull is Evil
Marc Branchaud wrote: What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You mean `git push main-repo HEAD:maintenance-branch`, right? -- Felipe Contreras -- 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: Pull is Evil
brian m. carlson wrote: At work, we have a workflow where we merge topic branches as non-fast-forward, so that we have a record of the history (including who reviewed the code), but when we want to just update our local branches, we always want fast-forward: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only If we make it the default, you only need to type `git pull`. git pull --no-ff developer-remote topic-branch I don't see anything wrong with having to type --no-ff if that's what you really want. git push main-repo HEAD main-repo/maintenance-branch should be the upstream of maintenance-branch, in which hase: % git push -- Felipe Contreras -- 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: Pull is Evil
On 14-05-01 03:22 PM, Felipe Contreras wrote: Marc Branchaud wrote: What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You mean `git push main-repo HEAD:maintenance-branch`, right? Right. Sorry, for that command I thoughtlessly just copied Brian's example. M. -- 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] Re: Pull is Evil
On Thu, May 01, 2014 at 02:16:50PM -0500, Felipe Contreras wrote: The only problem would be when it's not desirable, however, that's a problem of the user's ignorance, and the failure of the project's policity to communicate clearly to him that he should be running `git merge --no-ff`. There's absolutely nothing we can do to help him. I think “user ignorange” is the *only* problem with git pull. Once you understand the ff flags, you can set them however you like, and pull will do what you tell it to. The only thing we could do is not allow fast-forward merges either, in which case `git pull` becomes a no-op that can't possibly do anything ever. My interest in all of the proposed git-pull-training-wheel patches is that they give users a way to set a finger-breaking configuration that makes pull a no-op (or slows it down, like 'rm -i …'). Then folks who compulsively run 'git pull' (e.g. because SVN habits die slowly) can set an option that gives them something to think about before going ahead and running the pull anyway. The space in 'git pull' makes a shell-side: $ alias 'git pull'='echo try fetch/merge!' solution unfeasible, and clobbering /usr/libexec/git-core/git-pull seems a bit extreme. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [git] Re: Pull is Evil
On Thu, May 01, 2014 at 02:04:33PM -0400, Marc Branchaud wrote: On 14-05-01 01:56 PM, W. Trevor King wrote: On Thu, May 01, 2014 at 11:20:44AM -0400, Marc Branchaud wrote: On 14-05-01 05:46 AM, brian m. carlson wrote: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD … What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You're missing Brian's fast-forward merge here. It should be: git checkout maintenance-branch git fetch main-repo git merge --ff-only main-repo/maintenance-branch git fetch developer-remote … I think you're mistaken -- I checked out main-repo/maintenance-branch directly, so there's no need to fast-forward a local branch. I find a local branch useful to mark the amount of the upstream branch that I've reviewed. The reflog helps a bit, but I may go several fetches between reviews. For newbies, I recommend avoiding detached HEADs, where possible, so they don't have to rely on the reflog if they accidentally commit and then checkout something else (ignoring Git's warning). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [git] Re: Pull is Evil
On Thu, May 01, 2014 at 12:48:46PM -0700, W. Trevor King wrote: My interest in all of the proposed git-pull-training-wheel patches is that they give users a way to set a finger-breaking configuration that makes pull a no-op (or slows it down, like 'rm -i …'). Then folks who compulsively run 'git pull' (e.g. because SVN habits die slowly) can set an option that gives them something to think about before going ahead and running the pull anyway. Actually, what do we think about an -i/--interactive flag (with an associated pull.interactive boolean config to setup global/per-repo defaults)? Then after the fetch, you'd get one of the following: Merge $count commits from $repository $refspec into $current_branch? Rebase $count commits from $current_branch onto $repository $refpec? Fast-forward $current_branch by $count commits to $repository $refpec? and have a chance to bail out if you saw: Merge 1003 commits from git://example.net/main.git master into my-feature? because you forgot which branch you were on. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [git] Re: Pull is Evil
On 14-05-01 02:30 PM, W. Trevor King wrote: I find a local branch useful to mark the amount of the upstream branch that I've reviewed. The reflog helps a bit, but I may go several fetches between reviews. For newbies, I recommend avoiding detached HEADs, where possible, so they don't have to rely on the reflog if they accidentally commit and then checkout something else (ignoring Git's warning). All sound practices that I think are perfectly fine. I may be mistaken, but I think git pull evolved to try to address the detached-HEAD risk (at least in part). This risk was pretty real before the reflog came about (I'm under the impression -- and too lazy to check -- that git pull predates the reflog; please forgive me if I'm mis-perceiving the timeline). But these days there's hardly any risk to using a detached HEAD. Plus nowadays I think it's commonly accepted that using topic branches is a git best practice. The notion of doing work on a generically-named branch like maint seems archaic. So what benefit does git pull provide? In your particular case, you're using git pull to help you track your reviews of the upstream branch. To me this seems more like you taking advantage of a git pull side-effect than using the command as it is intended to be used. Certainly there are other ways that git can track this for you. A simple, aliasable, git tag -f LastReviewPoint upstream/branch seems just as effective to me (but then, I'm not you). M. -- 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 v6 02/42] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
In this case transaction is reset to NULL IFF ref_transaction_commit() was
invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
is safe. IF the conditional triggered prior to ref_transaction_commit()
then transaction is untouched and then ref_transaction_rollback(transaction)
will rollback the failed transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index 138ab70..2d83ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i transaction-nr; i++)
free(transaction-updates[i]);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 01/42] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 728a761..138ab70 100644
--- a/refs.c
+++ b/refs.c
@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 0f08def..892c5b6 100644
--- a/refs.h
+++ b/refs.h
@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 39/42] refs.c: add a new flag for transaction delete for refs we know are packed only
Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.
At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 19 +++
refs.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/refs.c b/refs.c
index 51cd41e..b525076 100644
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (transaction-status != REF_TRANSACTION_OPEN)
die(BUG: update on transaction that is not open);
+ if (flags REF_ISPACKONLY)
+ die(BUG: REF_ISPACKONLY can not be used with updates);
+
update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
@@ -3345,6 +3348,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
if (transaction-status != REF_TRANSACTION_OPEN)
die(BUG: create on transaction that is not open);
+ if (flags REF_ISPACKONLY)
+ die(BUG: REF_ISPACKONLY can not be used with creates);
+
update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
@@ -3458,10 +3464,20 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (ret)
goto cleanup;
+ for (i = 0; i n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update-flags REF_ISPACKONLY)
+ delnames[delnum++] = update-refname;
+ }
+
/* Acquire all locks while verifying old values */
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
+ if (update-flags REF_ISPACKONLY)
+ continue;
+
update-lock = lock_ref_sha1_basic(update-refname,
(update-have_old ?
update-old_sha1 :
@@ -3499,6 +3515,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
+ if (update-flags REF_ISPACKONLY)
+ continue;
+
if (update-lock) {
ret |= delete_ref_loose(update-lock, update-type);
if (!(update-flags REF_ISPRUNING))
diff --git a/refs.h b/refs.h
index 7a89415..71e39b9 100644
--- a/refs.h
+++ b/refs.h
@@ -136,6 +136,8 @@ extern int peel_ref(const char *refname, unsigned char
*sha1);
#define REF_NODEREF0x01
/** Deleting a loose ref during prune */
#define REF_ISPRUNING 0x02
+/** Deletion of a ref that only exists as a packed ref */
+#define REF_ISPACKONLY 0x04
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 21/42] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fetch.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..b41d7b7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv(GIT_REFLOG_ACTION);
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
if (dry_run)
return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
snprintf(msg, sizeof(msg), %s: %s, rla, action);
errno = 0;
- lock = lock_any_ref_for_update(ref-name,
- check_old ? ref-old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref-new_sha1, msg) 0)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old) ||
+ ref_transaction_commit(transaction, msg, NULL)) {
+ ref_transaction_rollback(transaction);
return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 19/42] refs.c: ref_transaction_commit should not free the transaction
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval)) ||
(ref_transaction_commit(t, action, err) !(t = NULL))) {
ref_transaction_rollback(t);
we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval) ||
ref_transaction_commit(t, action, err)) {
ref_transaction_rollback(t);
... die/return ...
ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 1 +
builtin/commit.c | 1 +
builtin/replace.c| 1 +
builtin/tag.c| 1 +
builtin/update-ref.c | 1 +
fast-import.c| 8
refs.c | 14 ++
refs.h | 14 +-
sequencer.c | 8
9 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/branch.c b/branch.c
index 2aa5c82..8e58908 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
ref_transaction_commit(transaction, msg, err))
die_errno(_(%s: failed to write ref: %s),
ref.buf, err.buf);
+ ref_transaction_free(transaction);
}
if (real_ref track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6117123..5bdcf9e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1686,6 +1686,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
rollback_index_files();
die(_(HEAD: cannot update ref: %s), err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path(CHERRY_PICK_HEAD));
unlink(git_path(REVERT_HEAD));
diff --git a/builtin/replace.c b/builtin/replace.c
index b037b29..5999f7d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -165,6 +165,7 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
ref_transaction_commit(transaction, NULL, err))
die(_(%s: failed to replace ref: %s), ref, err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/builtin/tag.c b/builtin/tag.c
index 1cf5650..d59154a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
0, !is_null_sha1(prev)) ||
ref_transaction_commit(transaction, NULL, err))
die(_(%s: cannot update the ref: %s), ref.buf, err.buf);
+ ref_transaction_free(transaction);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index fc3512f..7fe9c11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index 79d219b..3e356da 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1708,16 +1708,16 @@ static int update_branch(struct branch *b)
}
}
transaction = ref_transaction_begin();
- if ((!transaction ||
+ if (!transaction ||
ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
- 0, 1)) ||
- (ref_transaction_commit(transaction, msg, err)
-!(transaction = NULL))) {
+ 0, 1) ||
+ ref_transaction_commit(transaction, msg, err)) {
ref_transaction_rollback(transaction);
error(Unable to update branch %s: %s, b-name, err.buf);
strbuf_release(err);
return -1;
}
+ ref_transaction_free(transaction);
return 0;
}
diff --git a/refs.c b/refs.c
index c6ee43c..fcbdf9b 100644
--- a/refs.c
+++ b/refs.c
@@ -3302,7 +3302,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void
[PATCH v6 37/42] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 14d1573..8d82742 100644
--- a/refs.c
+++ b/refs.c
@@ -2043,6 +2043,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ return NULL;
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock-lock_fd = -1;
@@ -2134,8 +2137,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 41/42] refs.c: make rename_ref use a transaction
Change rename_ref to use a single transaction to perform the ref rename.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 73 ++
1 file changed, 20 insertions(+), 53 deletions(-)
diff --git a/refs.c b/refs.c
index eb75927..810a4db 100644
--- a/refs.c
+++ b/refs.c
@@ -2586,9 +2586,10 @@ static int rename_tmp_log(const char *newrefname)
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
+ unsigned char sha1[20];
+ int flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct stat loginfo;
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
@@ -2599,7 +2600,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (log S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
- symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, flag);
+ symref = resolve_ref_unsafe(oldrefname, sha1, 1, flag);
if (flag REF_ISSYMREF)
return error(refname %s is a symbolic ref, renaming it is not
supported,
oldrefname);
@@ -2621,62 +2622,28 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
return error(unable to pack refs);
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
- error(unable to delete old %s, oldrefname);
- goto rollback;
- }
-
- if (!read_ref_full(newrefname, sha1, 1, NULL)
- delete_ref(newrefname, sha1, REF_NODEREF)) {
- if (errno==EISDIR) {
- if (remove_empty_directories(git_path(%s,
newrefname))) {
- error(Directory not empty: %s, newrefname);
- goto rollback;
- }
- } else {
- error(unable to delete existing %s, newrefname);
- goto rollback;
- }
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, oldrefname, sha1,
+ REF_NODEREF | REF_ISPACKONLY,
+ 1, NULL) ||
+ ref_transaction_update(transaction, newrefname, sha1,
+ NULL, 0, 0, logmsg) ||
+ ref_transaction_commit(transaction, err)) {
+ ref_transaction_rollback(transaction);
+ error(rename_ref failed: %s, err.buf);
+ strbuf_release(err);
+ goto rollbacklog;
}
+ ref_transaction_free(transaction);
if (log rename_tmp_log(newrefname))
- goto rollback;
-
- logmoved = log;
-
- lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error(unable to lock %s for update, newrefname);
- goto rollback;
- }
- lock-force_write = 1;
- hashcpy(lock-old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
- error(unable to write current sha1 into %s, newrefname);
- goto rollback;
- }
-
- return 0;
-
- rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
- if (!lock) {
- error(unable to lock %s for rollback, oldrefname);
goto rollbacklog;
- }
- lock-force_write = 1;
- flag = log_all_ref_updates;
- log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
- error(unable to write current sha1 into %s, oldrefname);
- log_all_ref_updates = flag;
+ return 0;
rollbacklog:
- if (logmoved rename(git_path(logs/%s, newrefname),
git_path(logs/%s, oldrefname)))
- error(unable to restore logfile %s from %s: %s,
- oldrefname, newrefname, strerror(errno));
- if (!logmoved log
+ if (log
rename(git_path(TMP_RENAMED_LOG), git_path(logs/%s, oldrefname)))
error(unable to restore logfile %s from TMP_RENAMED_LOG: %s,
oldrefname, strerror(errno));
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 42/42] refs.c: remove forward declaraion of write_ref_sha1
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/refs.c b/refs.c
index 810a4db..e59bc18 100644
--- a/refs.c
+++ b/refs.c
@@ -251,8 +251,6 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
- const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 36/42] refs.c: pack all refs before we start to rename a ref
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c| 3 +++
t/t3200-branch.sh | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index e1f919e..14d1573 100644
--- a/refs.c
+++ b/refs.c
@@ -2595,6 +2595,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return error(unable to move logfile logs/%s to
TMP_RENAMED_LOG: %s,
oldrefname, strerror(errno));
+ if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+ return error(unable to pack refs);
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error(unable to delete old %s, oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master
test_must_fail git branch -m master2 master3
git symbolic-ref refs/heads/master2
- test_path_is_file .git/refs/heads/master
+ test_path_is_missing .git/refs/heads/master
test_path_is_missing .git/refs/heads/master3
'
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 22/42] fetch.c: use a single ref transaction for all ref updates
Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.
Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old cod
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.
This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.
For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
if (current_branch
!strcmp(ref-name, current_branch-name)
!(update_head_ok || is_bare_repository())
!is_null_sha1(ref-old_sha1)) {
/*
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
In the old code since we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the who
fetch with STORE_REF_ERROR_DF_CONFLICT
In the new code, since we defer committing the transaction until all refs
have been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update t
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/fetch.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b41d7b7..5dbd0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = ;
static const char *recurse_submodules_default;
static int shown_url;
+struct ref_transaction *transaction;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
- char msg[1024];
- char *rla = getenv(GIT_REFLOG_ACTION);
- struct ref_transaction *transaction;
-
if (dry_run)
return 0;
- if (!rla)
- rla = default_rla.buf;
- snprintf(msg, sizeof(msg), %s: %s, rla, action);
- errno = 0;
- transaction = ref_transaction_begin();
- if (!transaction ||
- ref_transaction_update(transaction, ref-name, ref-new_sha1,
- ref-old_sha1, 0, check_old) ||
- ref_transaction_commit(transaction, msg, NULL)) {
- ref_transaction_rollback(transaction);
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- }
- ref_transaction_free(transaction);
+ if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
+ ref-old_sha1, 0, check_old))
+ return STORE_REF_ERROR_OTHER;
+
return 0;
}
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
goto abort;
}
+ errno = 0;
+ transaction = ref_transaction_begin();
+ if (!transaction) {
+ rc = error(_(cannot start ref transaction\n));
+ goto abort;
+ }
+
/*
* We do a pass for each fetch_head_status type in their enum order, so
* merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
+ if (ref_transaction_commit(transaction, fetch_ref transaction, NULL))
+ rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+ STORE_REF_ERROR_OTHER;
+
[PATCH v6 38/42] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 8d82742..51cd41e 100644
--- a/refs.c
+++ b/refs.c
@@ -3462,12 +3462,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = lock_any_ref_for_update(update-refname,
- (update-have_old ?
- update-old_sha1 :
- NULL),
- update-flags,
- update-type);
+ update-lock = lock_ref_sha1_basic(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err ,Cannot lock the ref '%s'.,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 20/42] fetch.c: clear errno before calling functions that might set it
In s_update_ref there are two calls that when they fail we return an error based on the errno value. In particular we want to return a specific error if ENOTDIR happened. Both these functions do have failure modes where they may return an error without updating errno, in which case a previous and unrelated ENOTDIT may cause us to return the wrong error. Clear errno before calling any functions if we check errno afterwards. Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/fetch.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c..a93c893 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -44,7 +44,7 @@ static struct transport *gtransport; static struct transport *gsecondary; static const char *submodule_prefix = ; static const char *recurse_submodules_default; -static int shown_url = 0; +static int shown_url; static int option_parse_recurse_submodules(const struct option *opt, const char *arg, int unset) @@ -382,6 +382,8 @@ static int s_update_ref(const char *action, if (!rla) rla = default_rla.buf; snprintf(msg, sizeof(msg), %s: %s, rla, action); + + errno = 0; lock = lock_any_ref_for_update(ref-name, check_old ? ref-old_sha1 : NULL, 0, NULL); -- 2.0.0.rc1.351.g4d2c8e4 -- 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 v6 40/42] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to ckip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock anc create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 43 +--
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index b525076..eb75927 100644
--- a/refs.c
+++ b/refs.c
@@ -789,11 +789,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+ int i;
+ for(i = 0; i data-skipnum; i++) {
+ if (!strcmp(entry-name, data-skip[i])) {
+ return 0;
+ }
+ }
if (data-oldrefname !strcmp(data-oldrefname, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
@@ -808,15 +816,21 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
*/
static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2032,7 +2046,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2079,7 +2094,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache)))
{
+!is_refname_available(refname, NULL,
+ get_packed_refs(ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2137,7 +2154,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
+ if (!strcmp(oldrefname, newrefname))
+ return 0;
+
if (log S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
@@ -2586,10 +2606,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(ref_cache)))
+ if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(ref_cache), NULL, 0))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
[PATCH v6 23/42] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/receive-pack.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..d580176 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+struct strbuf err = STRBUF_INIT;
+struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd-old_sha1;
unsigned char *new_sha1 = cmd-new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) {
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
update_shallow_ref(cmd, si))
return shallow error;
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error(failed to lock %s, name);
- return failed to lock;
- }
- if (write_ref_sha1(lock, new_sha1, push)) {
- return failed to write; /* error() already called */
- }
+ if (ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1))
+ return failed to update;
return NULL; /* good */
}
}
@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
head_name = head_name_to_free = resolve_refdup(HEAD, sha1, 0, NULL);
checked_connectivity = 1;
+ transaction = ref_transaction_begin();
for (cmd = commands; cmd; cmd = cmd-next) {
if (cmd-error_string)
continue;
@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
+ if (ref_transaction_commit(transaction, push, err))
+ error(%s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
if (shallow_update !checked_connectivity)
error(BUG: run 'git fsck' for safety.\n
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 28/42] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 37 +
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index a82d6ba..206e1a2 100644
--- a/refs.c
+++ b/refs.c
@@ -3288,6 +3288,12 @@ struct ref_update {
const char refname[FLEX_ARRAY];
};
+enum ref_transaction_status {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
@@ -3297,6 +3303,7 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_status status;
};
struct ref_transaction *ref_transaction_begin(void)
@@ -3320,6 +3327,11 @@ void ref_transaction_free(struct ref_transaction
*transaction)
void ref_transaction_rollback(struct ref_transaction *transaction)
{
+ if (!transaction)
+ return;
+
+ transaction-status = REF_TRANSACTION_ERROR;
+
ref_transaction_free(transaction);
}
@@ -3344,7 +3356,10 @@ int ref_transaction_update(struct ref_transaction
*transaction,
struct ref_update *update;
if (have_old !old_sha1)
- die(have_old is true but old_sha1 is NULL);
+ die(BUG: have_old is true but old_sha1 is NULL);
+
+ if (transaction-status != REF_TRANSACTION_OPEN)
+ die(BUG: update on transaction that is not open);
update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
@@ -3363,7 +3378,10 @@ int ref_transaction_create(struct ref_transaction
*transaction,
struct ref_update *update;
if (!new_sha1 || is_null_sha1(new_sha1))
- die(create ref with null new_sha1);
+ die(BUG: create ref with null new_sha1);
+
+ if (transaction-status != REF_TRANSACTION_OPEN)
+ die(BUG: create on transaction that is not open);
update = add_update(transaction, refname);
@@ -3382,7 +3400,10 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
struct ref_update *update;
if (have_old !old_sha1)
- die(have_old is true but old_sha1 is NULL);
+ die(BUG: have_old is true but old_sha1 is NULL);
+
+ if (transaction-status != REF_TRANSACTION_OPEN)
+ die(BUG: delete on transaction that is not open);
update = add_update(transaction, refname);
update-flags = flags;
@@ -3454,8 +3475,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
- if (!n)
+ if (transaction-status != REF_TRANSACTION_OPEN)
+ die(BUG: commit on transaction that is not open);
+
+ if (!n) {
+ transaction-status = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3517,6 +3543,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(ref_cache);
cleanup:
+ transaction-status = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i n; i++)
if (updates[i]-lock)
unlock_ref(updates[i]-lock);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 26/42] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 4 +++-
refs.h | 3 ---
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index fcbdf9b..020eadf 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
};
static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+ const unsigned char *sha1, const char *logmsg);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
@@ -2798,7 +2800,7 @@ static int is_branch(const char *refname)
return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/);
}
-int write_ref_sha1(struct ref_lock *lock,
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index fcde43e..bc8a95b 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/** Setup reflog before using. **/
int log_ref_setup(const char *refname, char *logfile, int bufsize);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 10/42] refs.c: ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
status. Update all callers to check the return for ref_transaction_delete()
There are currently no conditions in _delete that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 5 +++--
refs.c | 15 ++-
refs.h | 8
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3fab810..fc3512f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,8 +257,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(delete %s: extra input: %s, refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old))
+ die(failed transaction delete for %s, refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 27db737..0a4e28e 100644
--- a/refs.c
+++ b/refs.c
@@ -3372,19 +3372,24 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old !old_sha1)
+ die(have_old is true but old_sha1 is NULL);
+
+ update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update-old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 01d773c..5eb500e 100644
--- a/refs.h
+++ b/refs.h
@@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 34/42] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 6 +++---
builtin/commit.c | 4 ++--
builtin/fetch.c| 10 --
builtin/receive-pack.c | 4 ++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 8
sequencer.c| 4 ++--
walker.c | 6 +++---
12 files changed, 60 insertions(+), 45 deletions(-)
diff --git a/branch.c b/branch.c
index 8e58908..74d55e7 100644
--- a/branch.c
+++ b/branch.c
@@ -300,9 +300,9 @@ void create_branch(const char *head,
transaction = ref_transaction_begin();
if (!transaction ||
- ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing) ||
- ref_transaction_commit(transaction, msg, err))
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, msg) ||
+ ref_transaction_commit(transaction, err))
die_errno(_(%s: failed to write ref: %s),
ref.buf, err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/commit.c b/builtin/commit.c
index 5bdcf9e..0f4e1fc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1681,8 +1681,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, HEAD, sha1,
current_head ?
current_head-object.sha1 : NULL,
- 0, !!current_head) ||
- ref_transaction_commit(transaction, sb.buf, err)) {
+ 0, !!current_head, sb.buf) ||
+ ref_transaction_commit(transaction, err)) {
rollback_index_files();
die(_(HEAD: cannot update ref: %s), err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5dbd0f0..3a849b0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
struct ref *ref,
int check_old)
{
+ char msg[1024];
+ char *rla = getenv(GIT_REFLOG_ACTION);
+
if (dry_run)
return 0;
+ if (!rla)
+ rla = default_rla.buf;
+ snprintf(msg, sizeof(msg), %s: %s, rla, action);
if (ref_transaction_update(transaction, ref-name, ref-new_sha1,
- ref-old_sha1, 0, check_old))
+ ref-old_sha1, 0, check_old, msg))
return STORE_REF_ERROR_OTHER;
return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
- if (ref_transaction_commit(transaction, fetch_ref transaction, NULL))
+ if (ref_transaction_commit(transaction, NULL))
rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
STORE_REF_ERROR_OTHER;
ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d580176..991c659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return shallow error;
if (ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1))
+ new_sha1, old_sha1, 0, 1, push))
return failed to update;
return NULL; /* good */
}
@@ -823,7 +823,7 @@ static void execute_commands(struct command *commands,
checked_connectivity = 0;
}
}
- if (ref_transaction_commit(transaction, push, err))
+ if (ref_transaction_commit(transaction, err))
error(%s, err.buf);
ref_transaction_free(transaction);
strbuf_release(err);
diff --git a/builtin/replace.c b/builtin/replace.c
index 5999f7d..47c360c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -161,8 +161,8 @@ static int replace_object(const char *object_ref, const
char *replace_ref,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0,
[PATCH v6 18/42] refs.c: free the transaction before returning when number of updates is 0
We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
t = ref_transaction_begin()
ref_transaction_commit(t)
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 563f336..c6ee43c 100644
--- a/refs.c
+++ b/refs.c
@@ -3451,8 +3451,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
- if (!n)
+ if (!n) {
+ ref_transaction_free(transaction);
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 29/42] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index 206e1a2..9f5435d 100644
--- a/refs.c
+++ b/refs.c
@@ -3235,24 +3235,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = Cannot lock the ref '%s'.;
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3496,12 +3478,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i n; i++) {
struct ref_update *update = updates[i];
- update-lock = update_ref_lock(update-refname,
- (update-have_old ?
- update-old_sha1 : NULL),
- update-flags,
- update-type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update-lock = lock_any_ref_for_update(update-refname,
+ (update-have_old ?
+ update-old_sha1 :
+ NULL),
+ update-flags,
+ update-type);
if (!update-lock) {
if (err)
strbuf_addf(err ,Cannot lock the ref '%s'.,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 27/42] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 020eadf..a82d6ba 100644
--- a/refs.c
+++ b/refs.c
@@ -2130,7 +2130,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index bc8a95b..b14fe86 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks a refs/ ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 33/42] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 31 ++-
1 file changed, 10 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 2b82dce..96b03e5 100644
--- a/refs.c
+++ b/refs.c
@@ -2481,11 +2481,6 @@ static int repack_without_refs(const char **refnames,
int n)
return commit_packed_refs();
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(refname, 1);
-}
-
static int delete_ref_loose(struct ref_lock *lock, int flag)
{
if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) {
@@ -2503,24 +2498,18 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, flag);
- if (!lock)
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 !is_null_sha1(sha1)) ||
+ ref_transaction_commit(transaction, NULL, NULL)) {
+ ref_transaction_rollback(transaction);
return 1;
- ret |= delete_ref_loose(lock, flag);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock-ref_name);
-
- unlink_or_warn(git_path(logs/%s, lock-ref_name));
- clear_loose_ref_cache(ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 32/42] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 22 +++---
refs.h | 2 ++
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index a55cc38..2b82dce 100644
--- a/refs.c
+++ b/refs.c
@@ -2330,17 +2330,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r-name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path(%s, r-name));
- unlock_ref(lock);
- try_remove_empty_parents(r-name);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_delete(transaction, r-name, r-sha1,
+ REF_ISPRUNING, 1) ||
+ ref_transaction_commit(transaction, NULL, err)) {
+ ref_transaction_rollback(transaction);
+ warning(prune_ref: %s, err.buf);
+ strbuf_release(err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r-name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3492,8 +3499,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update-lock) {
- delnames[delnum++] = update-lock-ref_name;
ret |= delete_ref_loose(update-lock, update-type);
+ if (!(update-flags REF_ISPRUNING))
+ delnames[delnum++] = update-lock-ref_name;
}
}
diff --git a/refs.h b/refs.h
index b14fe86..340e11a 100644
--- a/refs.h
+++ b/refs.h
@@ -134,6 +134,8 @@ extern int peel_ref(const char *refname, unsigned char
*sha1);
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
+/** Deleting a loose ref during prune */
+#define REF_ISPRUNING 0x02
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 31/42] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial refs/
part of the ref path name, the initial refs/ that this caller had already
stripped off before calling lock_ref_sha1.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 875178b..a55cc38 100644
--- a/refs.c
+++ b/refs.c
@@ -2130,15 +2130,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath(refs/%s, refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2339,8 +2330,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r-name + 5, r-sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r-name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path(%s, r-name));
unlock_ref(lock);
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 35/42] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 7e58693..e1f919e 100644
--- a/refs.c
+++ b/refs.c
@@ -2600,7 +2600,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, flag)
+ if (!read_ref_full(newrefname, sha1, 1, NULL)
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path(%s,
newrefname))) {
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 30/42] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 9f5435d..875178b 100644
--- a/refs.c
+++ b/refs.c
@@ -3235,25 +3235,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) 0) {
- const char *str = Cannot update the ref '%s'.;
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3498,14 +3479,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update-new_sha1)) {
- ret = update_ref_write(msg,
- update-refname,
- update-new_sha1,
- update-lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update-lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update-lock, update-new_sha1,
+msg);
+ update-lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = Cannot update the ref '%s'.;
+
+ if (err)
+ strbuf_addf(err, str, update-refname);
goto cleanup;
+ }
}
}
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 25/42] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
walker.c | 51 ++-
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..6044ccf 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error(Can't lock ref %s, write_ref[i]);
- goto unlock_and_fail;
- }
- }
+ transaction = ref_transaction_begin();
+ if (!transaction)
+ return -1;
if (!walker-get_recover)
for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i targets; i++) {
if (interpret_target(walker, target[i], sha1[20 * i])) {
error(Could not interpret response from server '%s' as
something to pull, target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch
(unknown));
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ sprintf(ref_name, refs/%s, write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name,
+ sha1[20 * i], NULL,
+ 0, 0))
+ goto rollback_and_fail;
+ }
+
+ if (ref_transaction_commit(transaction, msg ? msg : fetch (unknown),
+ err)) {
+ error(%s, err.buf);
+ goto rollback_and_fail;
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ free(msg);
+ strbuf_release(err);
+ ref_transaction_free(transaction);
return -1;
}
--
2.0.0.rc1.351.g4d2c8e4
--
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
[PATCH v6 16/42] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.
If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existsing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
branch.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..2aa5c82 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_(Not a valid branch point: '%s'.), start_name);
hashcpy(sha1, commit-object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_(Failed to lock ref for update));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, branch: Reset to %s,
start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, branch: Created from %s,
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing) ||
+ ref_transaction_commit(transaction, msg, err))
+ die_errno(_(%s: failed to write ref: %s),
+ ref.buf, err.buf);
+ }
+
if (real_ref track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) 0)
- die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
}
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 24/42] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 3e356da..5587cf6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1736,15 +1736,22 @@ static void dump_tags(void)
{
static const char *msg = fast-import;
struct tag *t;
- struct ref_lock *lock;
char ref_name[PATH_MAX];
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin();
for (t = first_tag; t; t = t-next_tag) {
- sprintf(ref_name, tags/%s, t-name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t-sha1, msg) 0)
- failure |= error(Unable to update %s, ref_name);
+ sprintf(ref_name, refs/tags/%s, t-name);
+
+ if (ref_transaction_update(transaction, ref_name, t-sha1,
+ NULL, 0, 0))
+ failure |= error(Unable to update %s, err.buf);
}
+ if (failure || ref_transaction_commit(transaction, msg, err))
+ failure |= error(Unable to update %s, err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(err);
}
static void dump_marks_helper(FILE *f,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 12/42] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/replace.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index b62420a..b037b29 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref, unsigned char object[20], prev[20], repl[20]; enum object_type obj_type, repl_type; char ref[PATH_MAX]; - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (get_sha1(object_ref, object)) die(Failed to resolve '%s' as a valid ref., object_ref); @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, else if (!force) die(replace ref '%s' already exists, ref); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die(%s: cannot lock the ref, ref); - if (write_ref_sha1(lock, repl, NULL) 0) - die(%s: cannot update the ref, ref); + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, + 0, !is_null_sha1(prev)) || + ref_transaction_commit(transaction, NULL, err)) + die(_(%s: failed to replace ref: %s), ref, err.buf); return 0; } -- 2.0.0.rc1.351.g4d2c8e4 -- 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 v6 11/42] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/tag.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index f344002..1cf5650 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_(%s: cannot lock the ref), ref.buf);
- if (write_ref_sha1(lock, object, NULL) 0)
- die(_(%s: cannot update the ref), ref.buf);
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, !is_null_sha1(prev)) ||
+ ref_transaction_commit(transaction, NULL, err))
+ die(_(%s: cannot update the ref: %s), ref.buf, err.buf);
if (force !is_null_sha1(prev) hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 17/42] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 0a4e28e..563f336 100644
--- a/refs.c
+++ b/refs.c
@@ -3396,11 +3396,28 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin();
+ if ((!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval)) ||
+ (ref_transaction_commit(t, action, err) !(t = NULL))) {
+ const char *str = update_ref failed for ref '%s': %s;
+
+ ref_transaction_rollback(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf); break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf); break;
+ case UPDATE_REFS_QUIET_ON_ERR: break;
+ }
+ strbuf_release(err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 14/42] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..9282a12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_(Failed to lock HEAD during fast_forward_to));
strbuf_addf(sb, %s: fast-forward, action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin();
+ if ((!transaction ||
+ ref_transaction_update(transaction, HEAD, to, from,
+ 0, !unborn)) ||
+ (ref_transaction_commit(transaction, sb.buf, err)
+!(transaction = NULL))) {
+ ref_transaction_rollback(transaction);
+ error(_(HEAD: Could not fast-forward: %s\n), err.buf);
+ strbuf_release(sb);
+ strbuf_release(err);
+ return -1;
+ }
strbuf_release(sb);
- return ret;
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 13/42] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/commit.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index f0b7906..6117123 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 !strcmp(argv[1], -h))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update(HEAD,
- !current_head
- ? NULL
- : current_head-object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_(cannot lock HEAD ref));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
@@ -1685,9 +1676,15 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) {
+ transaction = ref_transaction_begin();
+ if (!transaction ||
+ ref_transaction_update(transaction, HEAD, sha1,
+ current_head ?
+ current_head-object.sha1 : NULL,
+ 0, !!current_head) ||
+ ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
- die(_(cannot update HEAD ref));
+ die(_(HEAD: cannot update ref: %s), err.buf);
}
unlink(git_path(CHERRY_PICK_HEAD));
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 09/42] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
status. Update all callers to check the result of ref_transaction_create()
There are currently no conditions in _create that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 4 +++-
refs.c | 17 +++--
refs.h | 8
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 59c4d6b..3fab810 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags))
+ die(failed transaction create for %s, refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 1a903fb..27db737 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,18 +3353,23 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die(create ref with null new_sha1);
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 0364a3e..01d773c 100644
--- a/refs.h
+++ b/refs.h
@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* null SHA-1. It is verified that the reference does not exist
* already.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags);
/*
* Add a reference deletion to transaction. If have_old is true, then
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 00/42] Use ref transactions for all ref updates
This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 6: - Convert all updates in refs.c to use transactions too. Version 5: - Reword commit messages for having _create/_delete/_update returning success/failure. There are no conditions yet that return an error from these failures but there will be in the future. So we still check the return from these functions in the callers in preparation for this. - Don't leak memory by just passing a strbuf_detach() pointer to functions. Use obj.buf and explicitely strbuf_release the data afterwards. - Remove the function update_ref_lock. - Remove the function update_ref_write. - Track transaction status and die(BUG:) if we call _create/_delete/_update/ _commit for a transaction that is not OPEN. Version 4: - Rename patch series from Use ref transactions from most callers to Use ref transactions for all ref updates. - Convert all external ref writes to use transactions and make write_ref_sha1 and lock_ref_sha1 static functions. - Change the ref commit and free handling so we no longer pass pointer to pointer to _commit. _commit no longer frees the transaction. The caller MUST call _free itself. - Change _commit to take a strbuf pointer instead of a char* for error reporting back to the caller. - Re-add the walker patch after fixing it. Version 3: - Remove the walker patch for now. Walker needs more complex solution so defer it until the basics are done. - Remove the onerr argument to ref_transaction_commit(). All callers that need to die() on error now have to do this explicitely. - Pass an error string from ref_transaction_commit() back to the callers so that they can craft a nice error message upon failures. - Make ref_transaction_rollback() accept NULL as argument. - Change ref_transaction_commit() to take a pointer to pointer argument for the transaction and have it clear the callers pointer to NULL when invoked. This allows for much nicer handling of transaction rollback on failure. Version 2: - Add a patch to ref_transaction_commit to make it honor onerr even if the error triggered in ref_Transaction_commit itself rather than in a call to other functions (that already honor onerr). - Add a patch to make the update_ref() helper function use transactions internally. - Change ref_transaction_update to die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change ref_transaction_create to die() instead of error() if new_sha1 is false but we pass it a null_sha1. - Change ref_transaction_delete die() instead of error() if we pass if a NULL old_sha1 but have have_old == true. - Change several places to do if(!transaction || ref_transaction_update() || ref_Transaction_commit()) die(generic-message) instead of checking each step separately and having a different message for each failure. Most users are likely not interested in what step of the transaction failed and only whether it failed or not. - Change commit.c to only pass a pointer to ref_transaction_update iff current_head is non-NULL. The previous patch used to compute a garbage pointer for current_head-object.sha1 and relied on the fact that ref_transaction_update would not try to dereference this pointer if !!current_head was 0. - Updated commit message for the walker_fetch change to try to justify why the change in locking semantics should not be harmful. Ronnie Sahlberg (42): refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: make ref_update_reject_duplicates take a strbuf argument for errors update-ref.c: log transaction error from the update_ref refs.c: make update_ref_write update a strbuf on failure refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: ref_transaction_delete to check for error and return status tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use
[PATCH v6 07/42] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, err))
die(%s, err.buf);
return 0;
}
diff --git a/refs.c b/refs.c
index 5e06fd9..308e13e 100644
--- a/refs.c
+++ b/refs.c
@@ -3396,8 +3396,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i n; i++)
@@ -3407,22 +3406,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]-refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]-refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]-refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3437,7 +3427,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3449,7 +3439,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update-have_old ?
update-old_sha1 : NULL),
update-flags,
- update-type, onerr);
+ update-type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err)
strbuf_addf(err ,Cannot lock the ref '%s'.,
@@ -3467,7 +3458,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, err, onerr);
+ update-lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index ff87e14..bc7715e 100644
--- a/refs.h
+++ b/refs.h
@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 2d83ef6..64e8feb 100644
--- a/refs.c
+++ b/refs.c
@@ -3414,7 +3414,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update-flags,
update-type, onerr);
if (!update-lock) {
+ if (err)
+ strbuf_addf(err ,Cannot lock the ref '%s'.,
+ update-refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 892c5b6..ff87e14 100644
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem. The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 04/42] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 64e8feb..21a147b 100644
--- a/refs.c
+++ b/refs.c
@@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3400,6 +3401,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+ if (err)
+ strbuf_addf(err, str, updates[i]-refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3430,7 +3434,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 05/42] update-ref.c: log transaction error from the update_ref
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die(Refusing to perform update with empty message.);
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
- return ret;
+ if (ref_transaction_commit(transaction, msg, err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die(%s, err.buf);
+ return 0;
}
if (end_null)
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.
There are currently no conditions in _update that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
builtin/update-ref.c | 10 ++
refs.c | 9 +++--
refs.h | 10 +-
3 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..59c4d6b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old))
+ die(update %s: failed, refname);
update_flags = 0;
free(refname);
@@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old))
+ die(failed transaction update for %s, refname);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 308e13e..1a903fb 100644
--- a/refs.c
+++ b/refs.c
@@ -,19 +,24 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
int flags, int have_old)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old !old_sha1)
+ die(have_old is true but old_sha1 is NULL);
+ update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index bc7715e..0364a3e 100644
--- a/refs.h
+++ b/refs.h
@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old);
/*
* Add a reference creation to transaction. new_sha1 is the value
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 06/42] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 21a147b..5e06fd9 100644
--- a/refs.c
+++ b/refs.c
@@ -3253,10 +3253,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) 0) {
const char *str = Cannot update the ref '%s'.;
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3382,7 +3385,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3464,7 +3467,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update-refname,
update-new_sha1,
- update-lock, onerr);
+ update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.rc1.351.g4d2c8e4
--
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 v6 15/42] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
fast-import.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..79d219b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,45 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = fast-import;
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b-sha1)) {
if (b-delete)
delete_ref(b-name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
- if (!lock)
- return error(Unable to lock %s, b-name);
if (!force_update !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b-sha1, 0);
if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
return error(Branch %s is missing commits., b-name);
}
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning(Not updating %s
(new tip %s does not contain %s),
b-name, sha1_to_hex(b-sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b-sha1, msg) 0)
- return error(Unable to update %s, b-name);
+ transaction = ref_transaction_begin();
+ if ((!transaction ||
+ ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+ 0, 1)) ||
+ (ref_transaction_commit(transaction, msg, err)
+!(transaction = NULL))) {
+ ref_transaction_rollback(transaction);
+ error(Unable to update branch %s: %s, b-name, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
return 0;
}
--
2.0.0.rc1.351.g4d2c8e4
--
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 send-email doesn't work with IPv6 and STARTTLS
On 05/01/14 20:05, Jonathan Nieder wrote: Hi, Hi, Matthias-Christian Ott wrote[1]: git send-email uses Net::SMTP connections that use STARTTLS. Net::SMTP does not support IPv6. I patched Net:SMTP to use IO::Socket::INET6 and it worked. Thanks for reporting. 1. What version of Net::SMTP do you use? Included in Perl: Summary of my perl5 (revision 5 version 16 subversion 3) configuration: Platform: [...] hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: [...] intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: [...] libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc [...] Dynamic Linking: [...] Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES PERLIO_LAYERS PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP PERL_PRESERVE_IVUV USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO USE_PERL_ATOF Locally applied patches: gentoo/EUMM-RUNPATH - https://bugs.gentoo.org/105054 cpan/ExtUtils-MakeMaker: drop $PORTAGE_TMPDIR from LD_RUN_PATH gentoo/EUMM_delete_packlist - Don't install .packlist or perllocal.pod for perl or vendor gentoo/config_over - Remove -rpath and append LDFLAGS to lddlflags gentoo/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN. gentoo/cpanplus_definstalldirs - Configure CPANPLUS to use the site directories by default. gentoo/create_libperl_soname - https://bugs.gentoo.org/286840 Set libperl soname gentoo/drop_fstack_protector - https://bugs.gentoo.org/348557 Don't force -fstack-protector on everyone. gentoo/enc2xs - https://bugs.gentoo.org/338802 Tweak enc2xs to ignore missing @INC directories gentoo/mod_paths - Add /etc/perl to @INC gentoo/patchlevel - List packaged patches for perl-5.16.3(#1) in patchlevel.h gentoo/aix_soname - aix gcc detection and shared library soname support gentoo/solaris-relocation - Fix lddlflags for solaris gentoo/opensolars_headers - Add headers for opensolaris gentoo/cleanup-paths - Cleanup PATH and shrpenv gentoo/usr_local - Remove /usr/local paths gentoo/hints_hpux - Fix hpux hints gentoo/darwin-cc-ld - https://bugs.gentoo.org/297751 darwin: Use $CC to link gentoo/mint - [perl #89502] Mint fixes gentoo/interix - Fix interix hints fixes/maketext-code-execution - [1735f6f] http://bugs.gentoo.org/448632 Fix misparsing of maketext strings. fixes/cgi-cr-escaping - https://bugs.gentoo.org/443446 CR escaping for P3P and Set-Cookie headers debian/cpan-missing-site-dirs - https://bugs.gentoo.org/422981 Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable [...] 2. Would you be willing to contribute your patch to Net::SMTP upstream, for example by mailing it to bug-lib...@rt.cpan.org with the string [rt.cpan.org #93823] somewhere in the subject line[2]? I just did s/INET/INET6/g. I don't know Perl, so I guess Seffen Ullrich could help here more than I can. Regards Matthias-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
Re: git send-email doesn't work with IPv6 and STARTTLS
Hi, Steffen Ullrich wrote: git-send-mail does not use Net::SMTP directly for SSL support, but: - for direct connections (port 465) it uses Net::SMTP::SSL which just replaces the superclass if Net::SMTP with IO::Socket::SSL and thus implicitly supports IPv6 (because IO::Socket::SSL does) - for plain connections with SSL upgrade git-send-mail uses Net::SMTP for the initial connect and then does Net::SMTP::SSL-start_SSL (e.g. inherited from IO::Socket::SSL) to upgrade the socket to SSL. The problem here is that Net::SMTP does not support IPv6, but this should be solvable by using Net::INETGlue::INET_is_INET6 before loading Net::SMTP. Sounds like a good change, fwiw. Even after Net::SMTP is fixed, it would be useful as a way to make sure git works well with old versions of Net::SMTP too. But all these tricks are just workarounds for missing IPv6 and SSL support directly in the Net::SMTP, Net::FTP and Net::POP3. I therefore repeat my proposal from RT#93823 (no response yet) to add transparent support for IPv6 and SSL into these modules. By transparent I mean that the features are available if the necessary modules are installed (e.g. IO::Socket::SSL for SSL and IO::Socket::INET6 or IO::Socket::IP for IPv6), but that it works like before if they are not installed. I don't have these patches yet, but most of the necessary code is already there in Net::SSLGlue and Net::INET6Glue. Would you accept and incorporate such patches? Thanks for a kind offer. I am not Net::SMTP upstream, but I'd be happy to work with upstream to get such patches applied. Thanks for your work, Jonathan -- 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 send-email doesn't work with IPv6 and STARTTLS
Hi, Net::SMTP does neither supports SSL nor IPv6. git-send-mail does not use Net::SMTP directly for SSL support, but: - for direct connections (port 465) it uses Net::SMTP::SSL which just replaces the superclass if Net::SMTP with IO::Socket::SSL and thus implicitly supports IPv6 (because IO::Socket::SSL does) - for plain connections with SSL upgrade git-send-mail uses Net::SMTP for the initial connect and then does Net::SMTP::SSL-start_SSL (e.g. inherited from IO::Socket::SSL) to upgrade the socket to SSL. The problem here is that Net::SMTP does not support IPv6, but this should be solvable by using Net::INETGlue::INET_is_INET6 before loading Net::SMTP. But all these tricks are just workarounds for missing IPv6 and SSL support directly in the Net::SMTP, Net::FTP and Net::POP3. I therefore repeat my proposal from RT#93823 (no response yet) to add transparent support for IPv6 and SSL into these modules. By transparent I mean that the features are available if the necessary modules are installed (e.g. IO::Socket::SSL for SSL and IO::Socket::INET6 or IO::Socket::IP for IPv6), but that it works like before if they are not installed. I don't have these patches yet, but most of the necessary code is already there in Net::SSLGlue and Net::INET6Glue. Would you accept and incorporate such patches? Regards, Steffen On Thu, May 01, 2014 at 11:05:46AM -0700, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Matthias-Christian Ott wrote[1]: git send-email uses Net::SMTP connections that use STARTTLS. Net::SMTP does not support IPv6. I patched Net:SMTP to use IO::Socket::INET6 and it worked. Thanks for reporting. 1. What version of Net::SMTP do you use? 2. Would you be willing to contribute your patch to Net::SMTP upstream, for example by mailing it to bug-lib...@rt.cpan.org with the string [rt.cpan.org #93823] somewhere in the subject line[2]? Hope that helps, Jonathan [1] http://thread.gmane.org/gmane.comp.version-control.git/247769 [2] https://rt.cpan.org/Public/Bug/Display.html?id=93823 https://rt.cpan.org/NoAuth/RT/CPAN/Usage.html -- genua Gesellschaft fuer Netzwerk- und Unix-Administration mbH Domagkstrasse 7, 85551 Kirchheim bei Muenchen tel +49 89 991950-0, fax -999, www.genua.de Geschaeftsfuehrer: Dr. Magnus Harlander, Dr. Michaela Harlander, Bernhard Schneck. Amtsgericht Muenchen HRB 98238 -- 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: Pull is Evil
From: Marc Branchaud marcn...@xiplink.com Sent: Wednesday, April 30, 2014 8:45 PM [...] I don't think we'll ever be able to create a One Git Pull To Rule Them All. At best we'll end up with something with enough knobs that it could be configured to work in most workflows (I think we're actually pretty close to that). But for new users that defeats the purpose. It means that git pull is really an advanced command, and beginners should avoid it until they understand enough of git to configure it properly. So rather than perpetuate the myth that one command can always (or even just usually) do the right thing, let's just retire the command. All that said, I don't object to any attempts at improving the command either. But I also don't see any kind of improvement that would lead me to start using git pull let alone recommending it to new users. M. [1] By significant I mean enough to perpetually create new mailing list threads about changing 'git pull'. [general reply to all, rather than to anyone in particular, using Marc's summary] The point that there is no easy solution to an updated default pull action that is right for everybody, straight out of the box, I think is now fairly obvious, a summarised by Marc. I certainly avoid pull. My 'solution', if it could be called that, would be that at the point of switch over, after a period of release note warning and then code warning, that the plain 'git pull' would not even do the no-ff, but would simply refuse to do anything unless the user had explicitly set the [new] config variable(s) to a value of _their_ choice. The message could give guidance based on their old setting(s) and the new options as appropriate, i.e. if they have an old definitive setting then the new setting may be an obvious one. During the warning period between the release cycles, we may have a two step ramp up of the warning, where the first cycle allows users who have read the release notes to choose their new setting and it's auto detected from there on, then in the second cycle Git detects the lack of a setting and gives a warning prompt (just like the Git 2.0 warning), and finally the change over release makes a 'git pull' without a config setting an error. I know that for some it's a phaff that appears to waste time (been there, been that person), but it does allow the stragglers time to pick up the hints and not be too surprised, which will include many otherwise professional folks who just happen to have other priorities [e.g. this message typed from a Win XP machine!]. The approach does have a solid heritage, and avoids anyone (on the coding side) having to decide on an initial default, when it should be a user choice. Though I do agree with Filipe that the '--no-ff merge' would probably be the least worst for the new user and likely be a suitable 'if you don't know use this one' suggestion. 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
Re: Pull is Evil
Oops.. From: Philip Oakley philipoak...@iee.org From: Marc Branchaud marcn...@xiplink.com Sent: Wednesday, April 30, 2014 8:45 PM [...] I don't think we'll ever be able to create a One Git Pull To Rule Them All. At best we'll end up with something with enough knobs that it could be configured to work in most workflows (I think we're actually pretty close to that). But for new users that defeats the purpose. It means that git pull is really an advanced command, and beginners should avoid it until they understand enough of git to configure it properly. So rather than perpetuate the myth that one command can always (or even just usually) do the right thing, let's just retire the command. All that said, I don't object to any attempts at improving the command either. But I also don't see any kind of improvement that would lead me to start using git pull let alone recommending it to new users. M. [1] By significant I mean enough to perpetually create new mailing list threads about changing 'git pull'. [general reply to all, rather than to anyone in particular, using Marc's summary] The point that there is no easy solution to an updated default pull action that is right for everybody, straight out of the box, I think is now fairly obvious, a summarised by Marc. I certainly avoid pull. My 'solution', if it could be called that, would be that at the point of switch over, after a period of release note warning and then code warning, that the plain 'git pull' would not even do the no-ff, but s/no-ff/--ff/g that is, only 'merge' if it's a fast forward. would simply refuse to do anything unless the user had explicitly set the [new] config variable(s) to a value of _their_ choice. The message could give guidance based on their old setting(s) and the new options as appropriate, i.e. if they have an old definitive setting then the new setting may be an obvious one. During the warning period between the release cycles, we may have a two step ramp up of the warning, where the first cycle allows users who have read the release notes to choose their new setting and it's auto detected from there on, then in the second cycle Git detects the lack of a setting and gives a warning prompt (just like the Git 2.0 warning), and finally the change over release makes a 'git pull' without a config setting an error. I know that for some it's a phaff that appears to waste time (been there, been that person), but it does allow the stragglers time to pick up the hints and not be too surprised, which will include many otherwise professional folks who just happen to have other priorities [e.g. this message typed from a Win XP machine!]. The approach does have a solid heritage, and avoids anyone (on the coding side) having to decide on an initial default, when it should be a user choice. Though I do agree with Filipe that the '--no-ff merge' s/no-ff/--ff/ would probably be the least worst for the new user and likely be a suitable 'if you don't know use this one' suggestion. Philip -- sorry for the finger-brain failures. -- 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/8] CodingGuidelines: typofix
On Thu, May 01, 2014 at 10:51:19AM -0700, Junio C Hamano wrote: If you want to fix something here, do s/judgement/judgment/ instead. That too. FWIW, neither is outright wrong; it is an America/British variation, and apparently dictionaries disagree on which is preferred. -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
Re: [PATCH 7/8] CodingGuidelines: on comparison
On Wed, Apr 30, 2014 at 02:45:11PM -0700, Junio C Hamano wrote: See http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=4126 Signed-off-by: Junio C Hamano gits...@pobox.com Don't you often complain about submitters referencing a discussion in a commit message without providing some context or summary? + - There are two schools of thought when it comes to comparison, + especially inside a loop. Some people prefer to have less stable + value on the left hand side and more stable value on the right hand + side, e.g. if you have a loop that counts variable i down to the + lower bound, Grammar: /(less|more) stable/the / + Both are valid, and we use both, even though we tend to see the + former the more preferable, the more stable the more stable side + becomes (comparison with a constant, i 0, is an extreme + example). Just do not mix styles in the same part of the code. + I had trouble parsing the first sentence. Maybe: Both are valid, and we use both. However, the more stable the stable side becomes, the more we tend to prefer the former (comparison with a constant[...] -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
Re: [RFD] use approxidate for git commit --date=xyz?
On Wed, Apr 30, 2014 at 02:34:59PM -0700, Junio C Hamano wrote: Linus Torvalds torva...@linux-foundation.org writes: I just got a comment saying that git commit --amend --date=now doesn't work. I replied that you can use --date=$(date) Offhand without double-checking the actual codepath I do not have objection against approxidate-careful. This has come up a few times on the list, but nobody ever produced a patch. To quote myself[1]: I think the original rationale was that it's OK for us to allow some sloppiness when _viewing_ commits, since you will generally notice the problem. But when making commits, it's better to be careful, since you may be setting the sha1 in stone. These days we have two tools that could help: 1. approxidate_careful will do a regular approxidate, but keep track of whether we found anything even remotely useful. That doesn't mean you can't still get unexpected results, but at least some truly useless cases return errors. 2. For commits with a different author and committer, we mention the author name in the post-commit summary. We could do the same with a timestamp that was given (i.e., mentioning it in a standard format) to give the user another opportunity to double-check what we parsed. I think it would make sense if we followed both of those points. Should we also loosen $GIT_AUTHOR_DATE? I'd prefer not to, as that is not typically fed by the users themselves, but rather by scripts, and being robust there may be more valuable. But why does the workflow need --date=now in the first place? Wouldn't --reset-author do what you want better? What is the situation where you want to say that this patch has been changed significantly enough from the original to label it with the current timestamp without taking the authorship? In some of the instances on the list, the user simply didn't know that --reset-author would do the trick. And I do think it's slightly unintuitive. However, the original rationale for --date was to back-date commits[2], so even though there is an equivalent for --date=now, it might be nice to support --date=2.days.ago. -Peff [1] http://article.gmane.org/gmane.comp.version-control.git/168596 [2] http://article.gmane.org/gmane.comp.version-control.git/134406 -- 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
Zsh submodule name completion borked
When I use zsh tab-completion to complete the submodule name in 'git submodule init', I get more than I expected. From the gerrit repository (which has plugins): $ git submodule init plugins/TAB plugins/commit-message-length-validator\ \(v1.0-rc1-9-g545000b\) plugins/reviewnotes\ \(v1.0-rc1-8-ge984300\) plugins/replication\ \(v1.1-rc0-13-g4c3f4c9\) It works ok in bash. I tried to bisect the trouble, but it still fails in 1.8.3, so I'm beginning to think it's me. Does this happen to anyone else? Is it something in my local configuration causing this? Thanks, Phil -- 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: Zsh submodule name completion borked
Hi, Phil Hord wrote: When I use zsh tab-completion to complete the submodule name in 'git submodule init', I get more than I expected. Is this using zsh's native tab-completion (i.e., not the tab completion bundled with git)? There might have been a change there. Another place to look for hints would be ~/.gitconfig. Hope that helps, Jonathan -- 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: Zsh submodule name completion borked
Phil Hord wrote: When I use zsh tab-completion to complete the submodule name in 'git submodule init', I get more than I expected. From the gerrit repository (which has plugins): $ git submodule init plugins/TAB plugins/commit-message-length-validator\ \(v1.0-rc1-9-g545000b\) plugins/reviewnotes\ \(v1.0-rc1-8-ge984300\) plugins/replication\ \(v1.1-rc0-13-g4c3f4c9\) It works ok in bash. I tried to bisect the trouble, but it still fails in 1.8.3, so I'm beginning to think it's me. Does this happen to anyone else? Is it something in my local configuration causing this? Define 'works' in bash. From what I can see from the bash completion, it's not doing anything special, so the completion you see are simply files. -- Felipe Contreras -- 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] Define constants for lengths of object names
On Thu, May 01, 2014 at 10:20:07AM -0700, Jonathan Nieder wrote:
Hi,
brian m. carlson wrote:
--- a/object.h
+++ b/object.h
[...]
@@ -49,7 +56,7 @@ struct object {
unsigned used : 1;
unsigned type : TYPE_BITS;
unsigned flags : FLAG_BITS;
- unsigned char sha1[20];
+ unsigned char sha1[GIT_OID_RAWSZ];
Maybe my brain has been damaged by reading code from too many C
projects that hard-code some constants, but I find '20' easier to read
here.
What happened to the
struct object_id {
unsigned char id[20];
};
...
struct object {
...
struct object_id id;
};
idea?
There didn't seem to be a huge amount of support for it. Also, there
were concerns that some architectures might impose alignment constraints
on it that made sizeof(struct object_id) != 20.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
signature.asc
Description: Digital signature
Re: [PATCH] Define constants for lengths of object names
brian m. carlson wrote:
On Thu, May 01, 2014 at 10:20:07AM -0700, Jonathan Nieder wrote:
What happened to the
struct object_id {
unsigned char id[20];
};
...
struct object {
...
struct object_id id;
};
idea?
There didn't seem to be a huge amount of support for it.
I can make up for it in enthuasiasm. Please? It's something I've
wanted for a long time but never found the time to do.
Also, there
were concerns that some architectures might impose alignment constraints
on it that made sizeof(struct object_id) != 20.
Sounds awful. What architecture?
Thanks,
Jonathan
--
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] Re: Pull is Evil
W. Trevor King wrote: On Thu, May 01, 2014 at 02:16:50PM -0500, Felipe Contreras wrote: The only problem would be when it's not desirable, however, that's a problem of the user's ignorance, and the failure of the project's policity to communicate clearly to him that he should be running `git merge --no-ff`. There's absolutely nothing we can do to help him. I think “user ignorange” is the *only* problem with git pull. That, and the fact that 'git pull' does something wrong by default. The only thing we could do is not allow fast-forward merges either, in which case `git pull` becomes a no-op that can't possibly do anything ever. My interest in all of the proposed git-pull-training-wheel patches is that they give users a way to set a finger-breaking configuration that makes pull a no-op (or slows it down, like 'rm -i …'). Then folks who compulsively run 'git pull' (e.g. because SVN habits die slowly) can set an option that gives them something to think about before going ahead and running the pull anyway. The space in 'git pull' makes a shell-side: $ alias 'git pull'='echo try fetch/merge!' solution unfeasible, and clobbering /usr/libexec/git-core/git-pull seems a bit extreme. What is wrong with 'git pull' doing a merge when it can be fast-forward? -- Felipe Contreras-- 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] Re: Pull is Evil
W. Trevor King wrote: On Thu, May 01, 2014 at 12:48:46PM -0700, W. Trevor King wrote: My interest in all of the proposed git-pull-training-wheel patches is that they give users a way to set a finger-breaking configuration that makes pull a no-op (or slows it down, like 'rm -i …'). Then folks who compulsively run 'git pull' (e.g. because SVN habits die slowly) can set an option that gives them something to think about before going ahead and running the pull anyway. Actually, what do we think about an -i/--interactive flag (with an associated pull.interactive boolean config to setup global/per-repo defaults)? Then after the fetch, you'd get one of the following: Merge $count commits from $repository $refspec into $current_branch? Rebase $count commits from $current_branch onto $repository $refpec? Not much interactivity in those options. Maybe --prompt would make more sense. Fast-forward $current_branch by $count commits to $repository $refpec? Why would anyone say 'no' to this one? But your wording made me realize that my proposed option 'merge-ff-only' is not appropriate, because in theroy the user can think about it as 'rebase-ff-only'; in other words, a 'fast-forward' is not really a merge, and not really a rebase. and have a chance to bail out if you saw: Merge 1003 commits from git://example.net/main.git master into my-feature? because you forgot which branch you were on. Yes, that might be nice. But we still need to change the defaults. -- Felipe Contreras-- 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] Re: Pull is Evil
Marc Branchaud wrote: So what benefit does git pull provide? The same that 'hg update' provies: a way for the user fetch/pull the latest changes and check them out into the working directory. -- Felipe Contreras -- 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: Pull is Evil
Philip Oakley wrote: The point that there is no easy solution to an updated default pull action that is right for everybody, straight out of the box, I think is now fairly obvious, a summarised by Marc. I certainly avoid pull. Yes, I avoid it too, and quite a lot of people. My 'solution', if it could be called that, would be that at the point of switch over, after a period of release note warning and then code warning, that the plain 'git pull' would not even do the no-ff, but would simply refuse to do anything... I still haven't heard a single argument why a fast-forward by default wouldn't be desirable. Remember that we are talking about inexperienced users here. Experienced users can simply do `git pull --no-ff` or do the right configuration. The problem we want to track is newcomers doing merges (real ones) by mistake. Nobody ever complained about somebody doing a fast-forward by mistake. I think a non-fast-forward warning by default, and eventually rejecting them is the most sensible approach. -- Felipe Contreras -- 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] Re: Pull is Evil
On Thu, May 01, 2014 at 02:04:33PM -0400, Marc Branchaud wrote: On 14-05-01 01:56 PM, W. Trevor King wrote: On Thu, May 01, 2014 at 11:20:44AM -0400, Marc Branchaud wrote: On 14-05-01 05:46 AM, brian m. carlson wrote: git checkout maintenance-branch # Update our maintenance branch to the latest from the main repo. git pull --ff-only git pull --no-ff developer-remote topic-branch git push main-repo HEAD … What's more, it seems to me that the only real advantage git pull provides here is a less typing compared to the non-pull equivalent: git fetch main-repo git checkout main-repo/maintenance-branch git fetch developer-remote git merge --no-ff developer-remote/topic-branch git push main-repo HEAD You're missing Brian's fast-forward merge here. It should be: git checkout maintenance-branch git fetch main-repo git merge --ff-only main-repo/maintenance-branch git fetch developer-remote … I think you're mistaken -- I checked out main-repo/maintenance-branch directly, so there's no need to fast-forward a local branch. I actually need my local copy to be up-to-date. Part of my workflow, which I omitted for the sake of brevity, is running scripts that rely on my local branch's name, format, and contents. My use case is that I'm one of several code reviewers, and I update my branch, merge in another developer's changes, review them, and then push them if they're good. I need to pull from the main repo immediately before merging, to minimize the chances that someone else will have pushed before me, which would result in me having to redo the merge (because the push has to be fast-forward). I just used this to illustrate the fact that there isn't actually one completely correct case with pull. I have aliases for pull (and merge) --ff-only and --no-ff, and I never actually use plain git pull unless I really don't care whether or not it's a fast-forward. So I'm okay with the status quo because I have distinct choices for merge, no merge, and don't care. I don't really have a strong opinion, though, as long as those three options remain. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [git] Re: Pull is Evil
brian m. carlson wrote: I just used this to illustrate the fact that there isn't actually one completely correct case with pull. Nobody is arguing otherwise. The argument is that `git pull` by default can be made more sensible. -- Felipe Contreras -- 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: Pull is Evil
On Thu, May 01, 2014 at 06:34:06PM -0500, Felipe Contreras wrote: Nobody ever complained about somebody doing a fast-forward by mistake. Unless they fast-forward merged a feature branch into master, but the project prefers explicitly-merged feature branches with a cover-letter explaination in the merge commit [1]. On the one hand, folks integrating feature branches are likely more experienced Git users. On the other hand, I know several project maintainers who integrate feature branches that are pull-happy. I agree that accidental ff-merges are likely to be less troublesome than accidental non-ff merge/rebases, but I don't think changing the default to ff-only is a perfect fix. Cheers, Trevor [1]: http://article.gmane.org/gmane.comp.version-control.git/247807 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [git] Re: Pull is Evil
On Thu, May 01, 2014 at 06:25:16PM -0500, Felipe Contreras wrote: W. Trevor King wrote: On Thu, May 01, 2014 at 12:48:46PM -0700, W. Trevor King wrote: My interest in all of the proposed git-pull-training-wheel patches is that they give users a way to set a finger-breaking configuration that makes pull a no-op (or slows it down, like 'rm -i …'). Then folks who compulsively run 'git pull' (e.g. because SVN habits die slowly) can set an option that gives them something to think about before going ahead and running the pull anyway. Actually, what do we think about an -i/--interactive flag (with an associated pull.interactive boolean config to setup global/per-repo defaults)? Then after the fetch, you'd get one of the following: Merge $count commits from $repository $refspec into $current_branch? Rebase $count commits from $current_branch onto $repository $refpec? Not much interactivity in those options. Maybe --prompt would make more sense. I think matching rm, mv, cp, etc. is good, but I'd be ok with --prompt. Fast-forward $current_branch by $count commits to $repository $refpec? Why would anyone say 'no' to this one? Because the want explicit merges when they bring in topic branches? and have a chance to bail out if you saw: Merge 1003 commits from git://example.net/main.git master into my-feature? because you forgot which branch you were on. Yes, that might be nice. But we still need to change the defaults. So I should submit an orthogonal patch with -i/--interative/--prompt? Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[PATCH v6 2/7] pull: migrate all the tests to pull.mode
And branch.$name.pullmode. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- t/t5505-remote.sh | 2 +- t/t5520-pull.sh | 54 +++--- 2 files changed, 24 insertions(+), 32 deletions(-) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index ac79dd9..76376e4 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -181,7 +181,7 @@ test_expect_success 'show' ' git branch -d -r origin/master git config --add remote.two.url ../two git config --add remote.two.pushurl ../three - git config branch.rebase.rebase true + git config branch.rebase.pullmode rebase git config branch.octopus.merge topic-a topic-b topic-c ( cd ../one diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 227d293..01ad17a 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -123,26 +123,26 @@ test_expect_success '--rebase' ' test $(git rev-parse HEAD^) = $(git rev-parse copy) test new = $(git show HEAD:file2) ' -test_expect_success 'pull.rebase' ' +test_expect_success 'pull.mode=rebase' ' git reset --hard before-rebase - test_config pull.rebase true + test_config pull.mode rebase git pull . copy test $(git rev-parse HEAD^) = $(git rev-parse copy) test new = $(git show HEAD:file2) ' -test_expect_success 'branch.to-rebase.rebase' ' +test_expect_success 'branch.to-rebase.pullmode=rebase' ' git reset --hard before-rebase - test_config branch.to-rebase.rebase true + test_config branch.to-rebase.pullmode rebase git pull . copy test $(git rev-parse HEAD^) = $(git rev-parse copy) test new = $(git show HEAD:file2) ' -test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' +test_expect_success 'branch.to-rebase.pullmode should override pull.mode' ' git reset --hard before-rebase - test_config pull.rebase true - test_config branch.to-rebase.rebase false + test_config pull.mode merge + test_config branch.to-rebase.pullmode merge git pull . copy test $(git rev-parse HEAD^) != $(git rev-parse copy) test new = $(git show HEAD:file2) @@ -150,7 +150,7 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' # add a feature branch, keep-merge, that is merged into master, so the # test can try preserving the merge commit (or not) with various -# --rebase flags/pull.rebase settings. +# --rebase flags/pull.mode settings. test_expect_success 'preserve merge setup' ' git reset --hard before-rebase git checkout -b keep-merge second^ @@ -160,48 +160,40 @@ test_expect_success 'preserve merge setup' ' git tag before-preserve-rebase ' -test_expect_success 'pull.rebase=false create a new merge commit' ' +test_expect_success 'pull.mode=merge create a new merge commit' ' git reset --hard before-preserve-rebase - test_config pull.rebase false + test_config pull.mode merge git pull . copy test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) test $(git rev-parse HEAD^2) = $(git rev-parse copy) test file3 = $(git show HEAD:file3.t) ' -test_expect_success 'pull.rebase=true flattens keep-merge' ' +test_expect_success 'pull.mode=rebase flattens keep-merge' ' git reset --hard before-preserve-rebase - test_config pull.rebase true + test_config pull.mode rebase git pull . copy test $(git rev-parse HEAD^^) = $(git rev-parse copy) test file3 = $(git show HEAD:file3.t) ' -test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' ' +test_expect_success 'pull.mode=rebase-preserve rebases and merges keep-merge' ' git reset --hard before-preserve-rebase - test_config pull.rebase 1 - git pull . copy - test $(git rev-parse HEAD^^) = $(git rev-parse copy) - test file3 = $(git show HEAD:file3.t) -' - -test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' ' - git reset --hard before-preserve-rebase - test_config pull.rebase preserve + test_config pull.mode rebase-preserve git pull . copy test $(git rev-parse HEAD^^) = $(git rev-parse copy) test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge) ' -test_expect_success 'pull.rebase=invalid fails' ' +test_expect_success 'pull.mode=invalid fails' ' git reset --hard before-preserve-rebase - test_config pull.rebase invalid + test_config pull.mode invalid ! git pull . copy ' test_expect_success '--rebase=false create a new merge commit' ' git reset --hard before-preserve-rebase - test_config pull.rebase true + test_config pull.mode rebase git pull --rebase=false .
[PATCH v6 1/7] pull: rename pull.rebase to pull.mode
Also 'branch.name.rebase' to 'branch.name.pullmode'.
This way we can add more modes and the default can be something else,
namely it can be set to merge-ff-only, so eventually we can reject
non-fast-forward merges by default.
The old configurations still work, but get deprecated.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/config.txt | 39 ++-
Documentation/git-pull.txt | 2 +-
branch.c | 4 ++--
builtin/remote.c | 14 --
git-pull.sh| 31 +--
t/t3200-branch.sh | 40
t/t5601-clone.sh | 4 ++--
7 files changed, 88 insertions(+), 46 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c26a7c8..c028aeb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -708,7 +708,7 @@ branch.autosetupmerge::
branch.autosetuprebase::
When a new branch is created with 'git branch' or 'git checkout'
that tracks another branch, this variable tells Git to set
- up pull to rebase instead of merge (see branch.name.rebase).
+ up pull to rebase instead of merge (see branch.name.pullmode).
When `never`, rebase is never automatically set to true.
When `local`, rebase is set to true for tracked branches of
other local branches.
@@ -764,15 +764,17 @@ branch.name.mergeoptions::
option values containing whitespace characters are currently not
supported.
-branch.name.rebase::
- When true, rebase the branch name on top of the fetched branch,
- instead of merging the default branch from the default remote when
- git pull is run. See pull.rebase for doing this in a non
- branch-specific manner.
+branch.name.pullmode::
+ When git pull is run, this determines if it would either merge or
+ rebase the fetched branch. The possible values are 'merge',
+ 'rebase', and 'rebase-preserve'. See pull.mode for doing this in a
+ non branch-specific manner.
+
- When preserve, also pass `--preserve-merges` along to 'git rebase'
- so that locally committed merge commits will not be flattened
- by running 'git pull'.
+ When 'rebase-preserve', also pass `--preserve-merges` along to
+ 'git rebase' so that locally committed merge commits will not be
+ flattened by running 'git pull'.
++
+ It was named 'branch.name.rebase' but that is deprecated now.
+
*NOTE*: this is a possibly dangerous operation; do *not* use
it unless you understand the implications (see linkgit:git-rebase[1]
@@ -1881,15 +1883,18 @@ pretty.name::
Note that an alias with the same name as a built-in format
will be silently ignored.
-pull.rebase::
- When true, rebase branches on top of the fetched branch, instead
- of merging the default branch from the default remote when git
- pull is run. See branch.name.rebase for setting this on a
- per-branch basis.
+pull.mode::
+ When git pull is run, this determines if it would either merge or
+ rebase the fetched branch. The possible values are 'merge',
+ 'rebase', and 'rebase-preserve'. See branch.name.pullmode for doing
+ this in a non branch-specific manner.
++
+ When 'rebase-preserve', also pass `--preserve-merges` along to
+ 'git rebase' so that locally committed merge commits will not be
+ flattened by running 'git pull'.
++
+
- When preserve, also pass `--preserve-merges` along to 'git rebase'
- so that locally committed merge commits will not be flattened
- by running 'git pull'.
+ It was named 'pull.rebase' but that is deprecated now.
+
*NOTE*: this is a possibly dangerous operation; do *not* use
it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 200eb22..9a91b9f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -117,7 +117,7 @@ locally created merge commits will not be flattened.
+
When false, merge the current branch into the upstream branch.
+
-See `pull.rebase`, `branch.name.rebase` and `branch.autosetuprebase` in
+See `pull.mode`, `branch.name.pullmode` and `branch.autosetuprebase` in
linkgit:git-config[1] if you want to make `git pull` always use
`--rebase` instead of merging.
+
diff --git a/branch.c b/branch.c
index 723a36b..63ce671 100644
--- a/branch.c
+++ b/branch.c
@@ -71,8 +71,8 @@ void install_branch_config(int flag, const char *local, const
char *origin, cons
if (rebasing) {
strbuf_reset(key);
- strbuf_addf(key, branch.%s.rebase, local);
- git_config_set(key.buf, true);
+ strbuf_addf(key, branch.%s.pullmode, local);
+ git_config_set(key.buf, rebase);
}
[PATCH v6 0/7] Reject non-ff pulls by default
NOTE: Added a commit to throw a warning before the final switch.
It is very typical for Git newcomers to inadvertently create merges and worst:
inadvertently pushing them. This is one of the reasons many experienced users
prefer to avoid 'git pull', and recommend newcomers to avoid it as well.
To avoid these problems and keep 'git pull' useful, it has been agreed that
'git pull' should barf by default if the merge is non-fast-forward.
Unfortunately this breaks backwards-compatibility, so we need to be careful
about the error messages we give, and that we provide enough information to our
users to move forward without distrupting their workflow too much.
With the proper error messages and documentation, it has been agreed that the
new behavior is OK.
These are the steps needed to achieve this:
4) Only allow fast-forward merges by default
We could pass --ff-only to `git merge`, however, if we do that we'll get an
error like this:
Not possible to fast-forward, aborting.
This is not friendly; we want an error that is user-friendly:
The pull was not fast-forward, please either merge or rebase.
If unsure, run 'git pull --merge'.
When we do this we want to give the users the option to go back to the previous
behavior, so a new configuration is needed.
3) Add ff-only config
This option would trigger a check inside `git pull` itself, and error out with
the aforementioned message if it's not possible to do a fast-forward merge.
However, this option conflicts with --rebase, and --no-rebase. Solution below.
2) Add --merge option
Since we have a message that says If unsure, run 'git pull --merge', which is
more friendly than 'git pull --no-rebase', we should add this option, and
deprecate --no-rebase.
However, the documentation would become confusing if --merge is configured in
pull.rebase, instead, we want something like this:
See `pull.mode`, `branch.name.pullmode` in linkgit:git-config[1] if you want
to make `git pull` always use `--merge`.
1) Rename pull.rename to pull.mode and
branch.name.rebase to branch.name.pullmode
This way the configurations and options remain consistent:
git pull --merge
pull.mode = merge
branch.name.pullmode = merge
git pull --rebase
pull.mode = rebase
branch.name.pullmode = rebase
git pull --rebase=preserve
pull.mode = rebase-preserve
branch.name.pullmode = rebase-preserve
git pull
pull.mode = ff-only
branch.name.pullmode = ff-only
This patch series does all the steps mentioned, but in reverse order, and in
addition updates the tests to use the new configurations instead.
Changes since v5:
* Add commit to enable a transitional warning
* Renamed option to ff-only
* Added deprecation documentation for configs
* Removed config deprecation warnings
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4ebd3b5..b391ec1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -774,6 +774,8 @@ branch.name.pullmode::
'git rebase' so that locally committed merge commits will not be
flattened by running 'git pull'.
+
+ It was named 'branch.name.rebase' but that is deprecated now.
++
*NOTE*: this is a possibly dangerous operation; do *not* use
it unless you understand the implications (see linkgit:git-rebase[1]
for details).
@@ -1884,8 +1886,8 @@ pretty.name::
pull.mode::
When git pull is run, this determines if it would either merge or
rebase the fetched branch. The possible values are 'merge',
- 'rebase', 'merge-ff-only,' and 'rebase-preserve'.
- If 'merge-ff-only' is specified, the merge will only succeed if it's
+ 'rebase', 'ff-only,' and 'rebase-preserve'.
+ If 'ff-only' is specified, the merge will only succeed if it's
fast-forward.
See branch.name.pullmode for doing this in a non branch-specific
manner.
@@ -1894,6 +1896,9 @@ pull.mode::
'git rebase' so that locally committed merge commits will not be
flattened by running 'git pull'.
+
++
+ It was named 'pull.rebase' but that is deprecated now.
++
*NOTE*: this is a possibly dangerous operation; do *not* use
it unless you understand the implications (see linkgit:git-rebase[1]
for details).
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index ca8e951..968315c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -54,7 +54,7 @@ Then `git pull` will merge in a fast-foward way up to the new
master.
However, a non-fast-foward case looks very different.
- A---B---C origin/master
+ A---B---C master on origin
/
D---E---F---G master
^
diff --git a/git-pull.sh b/git-pull.sh
index 8cf8f68..bccaf27 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,7 +16,7 @@ cd_to_toplevel
warn () {
- printf 2 '%s\n' $*
+ printf 2 'warning: %s\n' $*
}
die_conflict () {
@@ -57,7 +57,7 @@ then
mode=$(git config pull.mode)
fi
case
[PATCH v6 3/7] pull: refactor $rebase variable into $mode
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
git-pull.sh | 64 +++--
1 file changed, 33 insertions(+), 31 deletions(-)
diff --git a/git-pull.sh b/git-pull.sh
index 3dbf9cf..50c612f 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -53,16 +53,11 @@ then
mode=$(git config pull.mode)
fi
case $mode in
-merge)
- rebase=false
- ;;
-rebase)
- rebase=true
+merge|rebase|'')
;;
rebase-preserve)
- rebase=preserve
- ;;
-'')
+ mode=rebase
+ rebase_args=--preserve-merges
;;
*)
echo Invalid value for 'mode'
@@ -71,7 +66,7 @@ rebase-preserve)
;;
esac
# backwards compatibility
-if test -z $rebase
+if test -z $mode
then
rebase=$(bool_or_string_config branch.$curr_branch_short.rebase)
if test -z $rebase
@@ -145,10 +140,10 @@ do
rebase=${1#*=}
;;
-r|--r|--re|--reb|--reba|--rebas|--rebase)
- rebase=true
+ mode=rebase
;;
--no-r|--no-re|--no-reb|--no-reba|--no-rebas|--no-rebase)
- rebase=false
+ mode=
;;
--recurse-submodules)
recurse_submodules=--recurse-submodules
@@ -179,19 +174,26 @@ do
shift
done
-case $rebase in
-preserve)
- rebase=true
- rebase_args=--preserve-merges
- ;;
-true|false|'')
- ;;
-*)
- echo Invalid value for --rebase, should be true, false, or preserve
- usage
- exit 1
- ;;
-esac
+if test -n $rebase
+then
+ case $rebase in
+ true)
+ mode=rebase
+ ;;
+ false)
+ mode=merge
+ ;;
+ preserve)
+ mode=rebase
+ rebase_args=--preserve-merges
+ ;;
+ *)
+ echo Invalid value for --rebase, should be true, false, or
preserve
+ usage
+ exit 1
+ ;;
+ esac
+fi
error_on_no_merge_candidates () {
exec 2
@@ -205,7 +207,7 @@ error_on_no_merge_candidates () {
esac
done
- if test true = $rebase
+ if test $mode = rebase
then
op_type=rebase
op_prep=against
@@ -218,7 +220,7 @@ error_on_no_merge_candidates () {
remote=$(git config branch.$curr_branch_short.remote)
if [ $# -gt 1 ]; then
- if [ $rebase = true ]; then
+ if [ $mode = rebase ]; then
printf There is no candidate for rebasing against
else
printf There are no candidates for merging
@@ -241,7 +243,7 @@ error_on_no_merge_candidates () {
exit 1
}
-test true = $rebase {
+test $mode = rebase {
if ! git rev-parse -q --verify HEAD /dev/null
then
# On an unborn branch
@@ -298,7 +300,7 @@ case $merge_head in
then
die $(gettext Cannot merge multiple branches into empty
head)
fi
- if test true = $rebase
+ if test $mode = rebase
then
die $(gettext Cannot rebase onto multiple branches)
fi
@@ -319,7 +321,7 @@ then
exit
fi
-if test true = $rebase
+if test $mode = rebase
then
o=$(git show-branch --merge-base $curr_branch $merge_head $oldremoteref)
if test $oldremoteref = $o
@@ -329,8 +331,8 @@ then
fi
merge_name=$(git fmt-merge-msg $log_arg $GIT_DIR/FETCH_HEAD) || exit
-case $rebase in
-true)
+case $mode in
+rebase)
eval=git-rebase $diffstat $strategy_args $merge_args $rebase_args
$verbosity
eval=$eval --onto $merge_head ${oldremoteref:-$merge_head}
;;
--
1.9.2+fc1.19.g85b6256
--
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 v6 5/7] pull: add merge-ff-only option
It is very typical for Git newcomers to inadvertently create merges and
worst; inadvertently pushing them. This is one of the reasons many
experienced users prefer to avoid 'git pull', and recommend newcomers to
avoid it as well.
To avoid these problems and keep 'git pull' useful, it has been
suggested that 'git pull' barfs by default if the merge is
non-fast-forward, which unfortunately would break backwards
compatibility.
This patch leaves everything in place to enable this new mode, but it
only gets enabled if the user specifically configures it; pull.mode =
merge-ff-only.
Later on this mode can be enabled by default.
For the full discussion you can read:
http://thread.gmane.org/gmane.comp.version-control.git/225146/focus=225305
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/config.txt | 7 +--
git-pull.sh | 15 ++-
t/t5520-pull.sh | 36
3 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c028aeb..b391ec1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1886,8 +1886,11 @@ pretty.name::
pull.mode::
When git pull is run, this determines if it would either merge or
rebase the fetched branch. The possible values are 'merge',
- 'rebase', and 'rebase-preserve'. See branch.name.pullmode for doing
- this in a non branch-specific manner.
+ 'rebase', 'ff-only,' and 'rebase-preserve'.
+ If 'ff-only' is specified, the merge will only succeed if it's
+ fast-forward.
+ See branch.name.pullmode for doing this in a non branch-specific
+ manner.
+
When 'rebase-preserve', also pass `--preserve-merges` along to
'git rebase' so that locally committed merge commits will not be
diff --git a/git-pull.sh b/git-pull.sh
index e7e52ec..2446417 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -57,7 +57,7 @@ then
mode=$(git config pull.mode)
fi
case $mode in
-merge|rebase|'')
+merge|rebase|ff-only|'')
;;
rebase-preserve)
mode=rebase
@@ -78,6 +78,7 @@ then
rebase=$(bool_or_string_config pull.rebase)
fi
fi
+test -z $mode mode=merge
dry_run=
while :
do
@@ -313,6 +314,18 @@ case $merge_head in
die $(gettext Cannot rebase onto multiple branches)
fi
;;
+*)
+ # check if a non-fast-foward merge would be needed
+ merge_head=${merge_head% }
+ if test $mode = 'ff-only' test -z
$no_ff$ff_only${squash#--no-squash}
+ test -n $orig_head
+ ! git merge-base --is-ancestor $orig_head $merge_head
+ ! git merge-base --is-ancestor $merge_head $orig_head
+ then
+ die $(gettext The pull was not fast-forward, please either
merge or rebase.
+If unsure, run 'git pull --merge'.)
+ fi
+ ;;
esac
# Pulling into unborn branch: a shorthand for branching off
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 01ad17a..a548c1b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -365,4 +365,40 @@ test_expect_success 'git pull --rebase against local
branch' '
test file = $(cat file2)
'
+test_expect_success 'git pull fast-forward (ff-only)' '
+ test_when_finished git checkout master git branch -D other test
+ test_config pull.mode ff-only
+ git checkout -b other master
+ new
+ git add new
+ git commit -m new
+ git checkout -b test -t other
+ git reset --hard master
+ git pull
+'
+
+test_expect_success 'git pull non-fast-forward (ff-only)' '
+ test_when_finished git checkout master git branch -D other test
+ test_config pull.mode ff-only
+ git checkout -b other master^
+ new
+ git add new
+ git commit -m new
+ git checkout -b test -t other
+ git reset --hard master
+ test_must_fail git pull
+'
+
+test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
+ test_when_finished git checkout master git branch -D other test
+ test_config pull.mode ff-only
+ git checkout -b other master^
+ new
+ git add new
+ git commit -m new
+ git checkout -b test -t other
+ git reset --hard master
+ git pull --merge
+'
+
test_done
--
1.9.2+fc1.19.g85b6256
--
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 v6 7/7] pull: only allow ff merges by default
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/git-pull.txt | 2 +-
git-pull.sh | 15 +++
t/t4013-diff-various.sh | 2 +-
t/t5500-fetch-pack.sh| 2 +-
t/t5520-pull.sh | 5 +
t/t5524-pull-msg.sh | 2 +-
t/t5700-clone-reference.sh | 4 ++--
t/t6022-merge-rename.sh | 20 ++--
t/t6026-merge-attr.sh| 2 +-
t/t6029-merge-subtree.sh | 6 +++---
t/t6037-merge-ours-theirs.sh | 10 +-
11 files changed, 29 insertions(+), 41 deletions(-)
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index fe3d15d..968315c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -61,7 +61,7 @@ However, a non-fast-foward case looks very different.
origin/master in your repository
-By default `git pull` will warn about these situations, however, most likely
+By default `git pull` will fail on these situations, however, most likely
you would want to force a merge, which you can do with `git pull --merge`.
Then `git pull` will fetch and replay the changes from the remote
diff --git a/git-pull.sh b/git-pull.sh
index c4a0b08..bccaf27 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -78,7 +78,7 @@ then
rebase=$(bool_or_string_config pull.rebase)
fi
fi
-test -z $mode mode=default
+test -z $mode mode=ff-only
dry_run=
while :
do
@@ -317,22 +317,13 @@ case $merge_head in
*)
# check if a non-fast-foward merge would be needed
merge_head=${merge_head% }
- if test -z $no_ff$ff_only${squash#--no-squash}
+ if test $mode = 'ff-only' test -z
$no_ff$ff_only${squash#--no-squash}
test -n $orig_head
! git merge-base --is-ancestor $orig_head $merge_head
! git merge-base --is-ancestor $merge_head $orig_head
then
- case $mode in
- ff-only)
- die $(gettext The pull was not fast-forward, please
either merge or rebase.
+ die $(gettext The pull was not fast-forward, please either
merge or rebase.
If unsure, run 'git pull --merge'.)
- ;;
- default)
- warn $(gettext the pull was not fast-forward, in the
future you would have to choose
-a merge or a rebase, falling back to old style for now (git pull --merge).
-Read 'git pull --help' for more information.) 2
- ;;
- esac
fi
;;
esac
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e77c09c..1840767 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -64,7 +64,7 @@ test_expect_success setup '
export GIT_AUTHOR_DATE GIT_COMMITTER_DATE
git checkout master
- git pull -s ours . side
+ git pull --merge -s ours . side
GIT_AUTHOR_DATE=2006-06-26 00:05:00 +
GIT_COMMITTER_DATE=2006-06-26 00:05:00 +
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 5b2b1c2..f735cfe 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -259,7 +259,7 @@ test_expect_success 'clone shallow object count' '
test_expect_success 'pull in shallow repo with missing merge base' '
(
cd shallow
- test_must_fail git pull --depth 4 .. A
+ test_must_fail git pull --merge --depth 4 .. A
)
'
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c96834e..dc7749b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -409,10 +409,7 @@ test_expect_success 'git pull non-fast-forward (default)' '
git commit -m new
git checkout -b test -t other
git reset --hard master
- git pull 2 error
- cat error
- grep -q the pull was not fast-forward error
- grep -q in the future you would have to choose error
+ test_must_fail git pull
'
test_done
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..ec9f413 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -25,7 +25,7 @@ test_expect_success setup '
test_expect_success pull '
(
cd cloned
- git pull --log
+ git pull --merge --log
git log -2
git cat-file commit HEAD result
grep Dollar result
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 6537911..306badf 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -94,7 +94,7 @@ cd $base_dir
test_expect_success 'pulling changes from origin' \
'cd C
-git pull origin'
+git pull --merge origin'
cd $base_dir
@@ -109,7 +109,7 @@ cd $base_dir
test_expect_success 'pulling changes from origin' \
'cd D
-git pull origin'
+git pull --merge origin'
cd $base_dir
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index a89dfbe..f63300f 100755
--- a/t/t6022-merge-rename.sh
+++
[PATCH v6 6/7] pull: add warning on non-ff merges
To prepare our uses for the upcoming changes we should warn them and let
them know that they will need to specify a merge or a rebase in the
future (when a non-fast-forward situation arises). Also, let them know
we fallback to 'git pull --merge', so when the obsoletion of this mode
comes, they know what to type without interrupting or changing their
workflow.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Documentation/git-pull.txt | 18 ++
git-pull.sh| 15 ---
t/t5520-pull.sh| 14 ++
3 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 767bca3..fe3d15d 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -23,6 +23,7 @@ More precisely, 'git pull' runs 'git fetch' with the given
parameters and calls 'git merge' to merge the retrieved branch
heads into the current branch.
With `--rebase`, it runs 'git rebase' instead of 'git merge'.
+With `--merge`, it forces the merge, even if it's non-fast forward.
repository should be the name of a remote repository as
passed to linkgit:git-fetch[1]. refspec can name an
@@ -41,11 +42,28 @@ Assume the following history exists and the current branch
is
A---B---C master on origin
/
+D---E master
+
+
+Then `git pull` will merge in a fast-foward way up to the new master.
+
+
+D---E---A---B---C master, origin/master
+
+
+However, a non-fast-foward case looks very different.
+
+
+ A---B---C master on origin
+/
D---E---F---G master
^
origin/master in your repository
+By default `git pull` will warn about these situations, however, most likely
+you would want to force a merge, which you can do with `git pull --merge`.
+
Then `git pull` will fetch and replay the changes from the remote
`master` branch since it diverged from the local `master` (i.e., `E`)
until its current commit (`C`) on top of `master` and record the
diff --git a/git-pull.sh b/git-pull.sh
index 2446417..c4a0b08 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -78,7 +78,7 @@ then
rebase=$(bool_or_string_config pull.rebase)
fi
fi
-test -z $mode mode=merge
+test -z $mode mode=default
dry_run=
while :
do
@@ -317,13 +317,22 @@ case $merge_head in
*)
# check if a non-fast-foward merge would be needed
merge_head=${merge_head% }
- if test $mode = 'ff-only' test -z
$no_ff$ff_only${squash#--no-squash}
+ if test -z $no_ff$ff_only${squash#--no-squash}
test -n $orig_head
! git merge-base --is-ancestor $orig_head $merge_head
! git merge-base --is-ancestor $merge_head $orig_head
then
- die $(gettext The pull was not fast-forward, please either
merge or rebase.
+ case $mode in
+ ff-only)
+ die $(gettext The pull was not fast-forward, please
either merge or rebase.
If unsure, run 'git pull --merge'.)
+ ;;
+ default)
+ warn $(gettext the pull was not fast-forward, in the
future you would have to choose
+a merge or a rebase, falling back to old style for now (git pull --merge).
+Read 'git pull --help' for more information.) 2
+ ;;
+ esac
fi
;;
esac
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a548c1b..c96834e 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -401,4 +401,18 @@ test_expect_success 'git pull non-fast-forward with merge
(ff-only)' '
git pull --merge
'
+test_expect_success 'git pull non-fast-forward (default)' '
+ test_when_finished git checkout master git branch -D other test
+ git checkout -b other master^
+ new
+ git add new
+ git commit -m new
+ git checkout -b test -t other
+ git reset --hard master
+ git pull 2 error
+ cat error
+ grep -q the pull was not fast-forward error
+ grep -q in the future you would have to choose error
+'
+
test_done
--
1.9.2+fc1.19.g85b6256
--
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
