[PATCH] builtin/remote.c: add missing space to user-facing message
Signed-off-by: Alex Henrie
---
builtin/remote.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..c55c7ce 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -13,7 +13,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote add [-t ] [-m ] [-f] [--tags|--no-tags]
[--mirror=] "),
N_("git remote rename "),
N_("git remote remove "),
- N_("git remote set-head (-a | --auto | -d | --delete
|)"),
+ N_("git remote set-head (-a | --auto | -d | --delete |
)"),
N_("git remote [-v | --verbose] show [-n] "),
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( |
)...]"),
--
2.1.3
--
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] builtin/remote.c: add missing space to user-facing message
On Wed, Dec 17, 2014 at 4:20 AM, Alex Henrie wrote:
> Signed-off-by: Alex Henrie
> ---
> builtin/remote.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 7f28f92..c55c7ce 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -13,7 +13,7 @@ static const char * const builtin_remote_usage[] = {
> N_("git remote add [-t ] [-m ] [-f]
> [--tags|--no-tags] [--mirror=] "),
This line also has a couple instance of missing spaces.
> N_("git remote rename "),
> N_("git remote remove "),
> - N_("git remote set-head (-a | --auto | -d | --delete
> |)"),
> + N_("git remote set-head (-a | --auto | -d | --delete |
> )"),
> N_("git remote [-v | --verbose] show [-n] "),
> N_("git remote prune [-n | --dry-run] "),
> N_("git remote [-v | --verbose] update [-p | --prune] [( |
> )...]"),
> --
> 2.1.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] remote: add --fetch and --both options to set-url
On 11/25/2014 12:48 PM, Peter Wu wrote:
git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for "update fetch URL and
leave whatever was in place for the push URL".
This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if no --both, --push or --fetch
options are given, then the push URL is modified too if it was not set
before. This is the case since the push URL is implicitly based on the
fetch URL.
A '--both' option is added to make the command independent of previous
pushurl settings. For the --add and --delete set operations, it will
always set the push and/ or the fetch URLs. For the primary mode of
operation (without --add or --delete), it will drop pushurl as the
implicit push URL is the (fetch) URL.
The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.
Tests are also added to verify the documented behavior.
Signed-off-by: Peter Wu
---
v2: fixed test case
v3: added --both option, changed --fetch --push behavior, added more tests for
--add/--delete cases, refactored to reduce duplication (not special-casing
add_mode without oldurl, just skip initially setting oldurl).
Translators note: the help text gained more translatable strings and some
strings got additional options.
---
Documentation/git-remote.txt | 17 --
builtin/remote.c | 140 +++
t/t5505-remote.sh| 125 ++
3 files changed, 228 insertions(+), 54 deletions(-)
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..bdd0305 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
'git remote remove'
'git remote set-head' (-a | --auto | -d | --delete | )
'git remote set-branches' [--add] ...
-'git remote set-url' [--push] []
-'git remote set-url --add' [--push]
-'git remote set-url --delete' [--push]
+'git remote set-url' [--both | --fetch | --push] []
+'git remote set-url' [--both | --fetch | --push] '--add'
+'git remote set-url' [--both | --fetch | --push] '--delete'
'git remote' [-v | --verbose] 'show' [-n] ...
'git remote prune' [-n | --dry-run] ...
'git remote' [-v | --verbose] 'update' [-p | --prune] [( |
)...]
@@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points
to matching
regex (first URL if no is given) to . If
doesn't match any URL, error occurs and nothing is changed.
+
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--both', both the fetch and push URLs are manipulated.
++
+With '--fetch', only fetch URLs are manipulated.
++
+With '--push', only push URLs are manipulated.
++
+If none of the '--both', '--fetch' or --push' options are given, then
+'--both' applies only if no push URL was set before. Otherwise '--fetch'
+is assumed for historical reasons. This default may change in the
+future to '--both' to avoid surprises depending on the configuration.
+
With '--add', instead of changing some URL, new URL is added.
+
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..a250b23 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( |
)...]"),
N_("git remote set-branches [--add] ..."),
- N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url [--both | --fetch | --push]
[]"),
+ N_("git remote set-url [--both | --fetch | --push] --add
"),
+ N_("git remote set-url [--both | --fetch | --push] --delete
"),
NULL
};
@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
};
static const char * const builtin_remote_seturl_usage[] = {
- N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url [--both | --fetch | --push]
[]"),
+ N_("git remote set-url [--both | --fetch | --push] --add
"),
+ N_("git remote set-url [--both | --fetch | --push] --delete
"),
NULL
};
@@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
}
+#define MODIFY_TYPE_FETCH (1 << 0)
+#define MODIFY_TYPE_PUSH(1 << 1)
+#define MODIFY_TYPE_BOTH(MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)
+#define MODIFY_TYPE_HISTORIC(MODIFY_TYPE_FETCH | (1 << 2))
+
static int set_url(int argc, const char **argv)
{
- int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+ int i, add_m
Improving git branch
Hi all, I'm interested in putting in some time and effort into improving the output of "git branch". What I'm thinking is an output like this: $ git branch 2014-12-17 * (detached from origin/master) deaba04 Do stuff 2014-12-15 john.ta/add_timing_info6edbcfa Add timing stuff 2014-12-14 master 8537316 [origin/master: ahead 1, behind 16] Some stuff 2014-12-12 john.ta/new_reduce_memory 99d84db Reintroduce: memory stuff 2014-12-05 john.ta/bugfixese15c95e Add stuff 2014-12-03 john.ta/container e9fd4e5 This branch is a test bed for containers (These columns are supposed to be all aligned nicely..) So, features: 1. Show the date of the last commit 2. Sort by date. Most recently used branches at the top 3. Show the branch name, including your current "branch", with a * to indicate that it's checked out. 4. Show the sha 5. Show the branch DESCRIPTION - and if that's not available, show the short-line of the most recent commit. There is also a small amount of color here that I can't paste here, to follow the coloring in the current git branch. Before I start making patches etc, what do people think? Would I have a chance of getting this in? Should I change some aspects etc? Thanks, John Tapsell -- 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: Improving git branch
John Tapsell schrieb am 17.12.2014 um 12:10: > Hi all, > > I'm interested in putting in some time and effort into improving the > output of "git branch". > > What I'm thinking is an output like this: > > $ git branch > > 2014-12-17 * (detached from origin/master) deaba04 Do stuff > 2014-12-15 john.ta/add_timing_info6edbcfa Add timing stuff > 2014-12-14 master 8537316 > [origin/master: ahead 1, behind 16] Some stuff > 2014-12-12 john.ta/new_reduce_memory 99d84db Reintroduce: memory stuff > 2014-12-05 john.ta/bugfixese15c95e Add stuff > 2014-12-03 john.ta/container e9fd4e5 This > branch is a test bed for containers > > > (These columns are supposed to be all aligned nicely..) > > So, features: > > 1. Show the date of the last commit > 2. Sort by date. Most recently used branches at the top > 3. Show the branch name, including your current "branch", with a * to > indicate that it's checked out. > 4. Show the sha > 5. Show the branch DESCRIPTION - and if that's not available, show the > short-line of the most recent commit. > > There is also a small amount of color here that I can't paste here, to > follow the coloring in the current git branch. > > Before I start making patches etc, what do people think? Would I have > a chance of getting this in? Should I change some aspects etc? > > Thanks, > > John Tapsell > I support the general goal, we have quite some way to go there. As to the method: "git branch" in list mode, "git tag" in list mode and "git for-each-ref" all do similar things and are in turn not dissimilar from "git log --no-walk" with appropriate formatting and rev options. Rather than extending "git branch" any further[*], I suggest a bolder strategy: - unify/merge for-each-ref and pretty formats (and code) as far as possible - leverage that for the list modes of branch and tag That would allow everyone to get their favourite listing, just like for logs. Otherwise it would be very difficult to agree on *the* proper format for an extended branch or tag list. Michael [*] I know I'm a sinner, 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: Improving git branch
I don't fully understand - if I did that, then what difference would an average user actually see? On 17 December 2014 at 11:28, Michael J Gruber wrote: > John Tapsell schrieb am 17.12.2014 um 12:10: >> Hi all, >> >> I'm interested in putting in some time and effort into improving the >> output of "git branch". >> >> What I'm thinking is an output like this: >> >> $ git branch >> >> 2014-12-17 * (detached from origin/master) deaba04 Do stuff >> 2014-12-15 john.ta/add_timing_info6edbcfa Add timing stuff >> 2014-12-14 master 8537316 >> [origin/master: ahead 1, behind 16] Some stuff >> 2014-12-12 john.ta/new_reduce_memory 99d84db Reintroduce: memory >> stuff >> 2014-12-05 john.ta/bugfixese15c95e Add stuff >> 2014-12-03 john.ta/container e9fd4e5 This >> branch is a test bed for containers >> >> >> (These columns are supposed to be all aligned nicely..) >> >> So, features: >> >> 1. Show the date of the last commit >> 2. Sort by date. Most recently used branches at the top >> 3. Show the branch name, including your current "branch", with a * to >> indicate that it's checked out. >> 4. Show the sha >> 5. Show the branch DESCRIPTION - and if that's not available, show the >> short-line of the most recent commit. >> >> There is also a small amount of color here that I can't paste here, to >> follow the coloring in the current git branch. >> >> Before I start making patches etc, what do people think? Would I have >> a chance of getting this in? Should I change some aspects etc? >> >> Thanks, >> >> John Tapsell >> > > I support the general goal, we have quite some way to go there. > > As to the method: "git branch" in list mode, "git tag" in list mode and > "git for-each-ref" all do similar things and are in turn not dissimilar > from "git log --no-walk" with appropriate formatting and rev options. > > Rather than extending "git branch" any further[*], I suggest a bolder > strategy: > > - unify/merge for-each-ref and pretty formats (and code) as far as possible > - leverage that for the list modes of branch and tag > > That would allow everyone to get their favourite listing, just like for > logs. Otherwise it would be very difficult to agree on *the* proper > format for an extended branch or tag list. > > Michael > > > [*] I know I'm a sinner, 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: Improving git branch
Also, please don't top-post here. That would allow everyone to get their favourite listing, just like for logs. John Tapsell schrieb am 17.12.2014 um 12:51: > I don't fully understand - if I did that, then what difference would > an average user actually see? > > On 17 December 2014 at 11:28, Michael J Gruber > wrote: >> John Tapsell schrieb am 17.12.2014 um 12:10: >>> Hi all, >>> >>> I'm interested in putting in some time and effort into improving the >>> output of "git branch". >>> >>> What I'm thinking is an output like this: >>> >>> $ git branch >>> >>> 2014-12-17 * (detached from origin/master) deaba04 Do stuff >>> 2014-12-15 john.ta/add_timing_info6edbcfa Add timing >>> stuff >>> 2014-12-14 master 8537316 >>> [origin/master: ahead 1, behind 16] Some stuff >>> 2014-12-12 john.ta/new_reduce_memory 99d84db Reintroduce: memory >>> stuff >>> 2014-12-05 john.ta/bugfixese15c95e Add stuff >>> 2014-12-03 john.ta/container e9fd4e5 This >>> branch is a test bed for containers >>> >>> >>> (These columns are supposed to be all aligned nicely..) >>> >>> So, features: >>> >>> 1. Show the date of the last commit >>> 2. Sort by date. Most recently used branches at the top >>> 3. Show the branch name, including your current "branch", with a * to >>> indicate that it's checked out. >>> 4. Show the sha >>> 5. Show the branch DESCRIPTION - and if that's not available, show the >>> short-line of the most recent commit. >>> >>> There is also a small amount of color here that I can't paste here, to >>> follow the coloring in the current git branch. >>> >>> Before I start making patches etc, what do people think? Would I have >>> a chance of getting this in? Should I change some aspects etc? >>> >>> Thanks, >>> >>> John Tapsell >>> >> >> I support the general goal, we have quite some way to go there. >> >> As to the method: "git branch" in list mode, "git tag" in list mode and >> "git for-each-ref" all do similar things and are in turn not dissimilar >> from "git log --no-walk" with appropriate formatting and rev options. >> >> Rather than extending "git branch" any further[*], I suggest a bolder >> strategy: >> >> - unify/merge for-each-ref and pretty formats (and code) as far as possible >> - leverage that for the list modes of branch and tag >> >> That would allow everyone to get their favourite listing, just like for >> logs. Otherwise it would be very difficult to agree on *the* proper >> format for an extended branch or tag list. >> >> Michael >> >> >> [*] I know I'm a sinner, 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
[PATCH v4] remote: add --fetch and --both options to set-url
git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for "update fetch URL and
leave whatever was in place for the push URL".
This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if no --both, --push or --fetch
options are given, then the push URL is modified too if it was not set
before. This is the case since the push URL is implicitly based on the
fetch URL.
A '--both' option is added to make the command independent of previous
pushurl settings. For the --add and --delete set operations, it will
always set the push and/ or the fetch URLs. For the primary mode of
operation (without --add or --delete), it will drop pushurl as the
implicit push URL is the (fetch) URL.
While '--both' could be implemented as '--fetch --push', it might also
be mistaken for "--push overrides --fetch". An option such as
"--only={fetch|push|both}" was also considered, but it is longer than
the current options, makes --push redundant and brings the confusing
option "--only=both". Options such as '--direction=...' is too long and
'--dir=' is ambiguous ("directory"?). Thus, for brevity three new
options were introduced.
The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.
Tests are also added to verify the documented behavior.
Signed-off-by: Peter Wu
---
Hi,
This is the fourth revision of the patch that allows for just setting the fetch
URL. Eric wondered why '--fetch --push' is not used to describe the state
'--both', so I added this to the commit message.
The t5505-remote.sh test still passes after this change (I was unable to run the
full test suite as it broke due to an unrelated gpg issue).
Kind regards,
Peter
v2: fixed test case
v3: added --both option, changed --fetch --push behavior, added more tests for
--add/--delete cases, refactored to reduce duplication (not special-casing
add_mode without oldurl, just skip initially setting oldurl).
v4: incorporated documentation suggestion from Eric Sunshine;
Tried to make the code easier to follow by replacing
(modify_type == MODIFY_TYPE_FETCH) by
(modify_type & MODIFY_TYPE_FETCH && modify_type != MODIFY_TYPE_HISTORIC)
and added comments explaining why this is special (observed by Jeff King);
Fixed dangling else issue reported by Torsten Bögershausen;
Added considerations for other options to the commit message;
Rebased on v2.2.0-65-g9abc44b.
---
Documentation/git-remote.txt | 16 +++--
builtin/remote.c | 146 ---
t/t5505-remote.sh| 125
3 files changed, 234 insertions(+), 53 deletions(-)
diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..09a4670 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
'git remote remove'
'git remote set-head' (-a | --auto | -d | --delete | )
'git remote set-branches' [--add] ...
-'git remote set-url' [--push] []
-'git remote set-url --add' [--push]
-'git remote set-url --delete' [--push]
+'git remote set-url' [--both | --fetch | --push] []
+'git remote set-url' [--both | --fetch | --push] '--add'
+'git remote set-url' [--both | --fetch | --push] '--delete'
'git remote' [-v | --verbose] 'show' [-n] ...
'git remote prune' [-n | --dry-run] ...
'git remote' [-v | --verbose] 'update' [-p | --prune] [( | )...]
@@ -134,7 +134,15 @@ Changes URL remote points to. Sets first URL remote points
to matching
regex (first URL if no is given) to . If
doesn't match any URL, error occurs and nothing is changed.
+
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--both', both the fetch and push URLs are manipulated.
++
+With '--fetch', only fetch URLs are manipulated.
++
+With '--push', only push URLs are manipulated.
++
+For historical reasons, if neither --fetch nor --push is specified then the
+fetch URL is changed, as well as the push URL if this was not already set. This
+behavior may change in the future.
+
With '--add', instead of changing some URL, new URL is added.
+
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..1fa2fd7 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( |
)...]"),
N_("git remote set-branches [--add] ..."),
- N_("git remote set-url [--push] []"),
- N_("git remote set-url --add "),
- N_("git remote set-url --delete "),
+ N_("git remote set-url [--both | --fetch | --push]
[]"),
+ N_("git remote set-url [--both | --fetch | --push] --add
"),
+ N_("git remote set-url [--both | --fetch | --push] --delete
"),
NULL
Re: [PATCH v3] remote: add --fetch and --both options to set-url
On Wednesday 17 December 2014 11:08:07 Torsten Bögershausen wrote:
> On 11/25/2014 12:48 PM, Peter Wu wrote:
> > git remote set-url knew about the '--push' option to update just the
> > pushurl, but it does not have a similar option for "update fetch URL and
> > leave whatever was in place for the push URL".
> >
> > This patch adds support for a '--fetch' option which implements that use
> > case in a backwards compatible way: if no --both, --push or --fetch
> > options are given, then the push URL is modified too if it was not set
> > before. This is the case since the push URL is implicitly based on the
> > fetch URL.
> >
> > A '--both' option is added to make the command independent of previous
> > pushurl settings. For the --add and --delete set operations, it will
> > always set the push and/ or the fetch URLs. For the primary mode of
> > operation (without --add or --delete), it will drop pushurl as the
> > implicit push URL is the (fetch) URL.
> >
> > The documentation has also been updated and a missing '--push' option
> > is added to the 'git remote -h' command.
> >
> > Tests are also added to verify the documented behavior.
> >
> > Signed-off-by: Peter Wu
> > ---
> >
> > v2: fixed test case
> > v3: added --both option, changed --fetch --push behavior, added more
> > tests for
> > --add/--delete cases, refactored to reduce duplication (not
> > special-casing
> > add_mode without oldurl, just skip initially setting oldurl).
> >
> > Translators note: the help text gained more translatable strings and some
> > strings got additional options.
> > ---
> > Documentation/git-remote.txt | 17 --
> > builtin/remote.c | 140
> > +++
> > t/t5505-remote.sh| 125 ++
> > 3 files changed, 228 insertions(+), 54 deletions(-)
> >
> > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> > index cb103c8..bdd0305 100644
> > --- a/Documentation/git-remote.txt
> > +++ b/Documentation/git-remote.txt
> > @@ -15,9 +15,9 @@ SYNOPSIS
> > 'git remote remove'
> > 'git remote set-head' (-a | --auto | -d | --delete | )
> > 'git remote set-branches' [--add] ...
> > -'git remote set-url' [--push] []
> > -'git remote set-url --add' [--push]
> > -'git remote set-url --delete' [--push]
> > +'git remote set-url' [--both | --fetch | --push] []
> > +'git remote set-url' [--both | --fetch | --push] '--add'
> > +'git remote set-url' [--both | --fetch | --push] '--delete'
> > 'git remote' [-v | --verbose] 'show' [-n] ...
> > 'git remote prune' [-n | --dry-run] ...
> > 'git remote' [-v | --verbose] 'update' [-p | --prune] [( |
> > )...]
> > @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote
> > points to matching
> > regex (first URL if no is given) to . If
> >doesn't match any URL, error occurs and nothing is changed.
> > +
> > -With '--push', push URLs are manipulated instead of fetch URLs.
> > +With '--both', both the fetch and push URLs are manipulated.
> > ++
> > +With '--fetch', only fetch URLs are manipulated.
> > ++
> > +With '--push', only push URLs are manipulated.
> > ++
> > +If none of the '--both', '--fetch' or --push' options are given, then
> > +'--both' applies only if no push URL was set before. Otherwise '--fetch'
> > +is assumed for historical reasons. This default may change in the
> > +future to '--both' to avoid surprises depending on the configuration.
> > +
> > With '--add', instead of changing some URL, new URL is added.
> > +
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 7f28f92..a250b23 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
> > N_("git remote prune [-n | --dry-run] "),
> > N_("git remote [-v | --verbose] update [-p | --prune] [( |
> > )...]"),
> > N_("git remote set-branches [--add] ..."),
> > - N_("git remote set-url [--push] []"),
> > - N_("git remote set-url --add "),
> > - N_("git remote set-url --delete "),
> > + N_("git remote set-url [--both | --fetch | --push]
> > []"),
> > + N_("git remote set-url [--both | --fetch | --push] --add
> > "),
> > + N_("git remote set-url [--both | --fetch | --push] --delete
> > "),
> > NULL
> > };
> >
> > @@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] =
> > {
> > };
> >
> > static const char * const builtin_remote_seturl_usage[] = {
> > - N_("git remote set-url [--push] []"),
> > - N_("git remote set-url --add "),
> > - N_("git remote set-url --delete "),
> > + N_("git remote set-url [--both | --fetch | --push]
> > []"),
> > + N_("git remote set-url [--both | --fetch | --push] --add
> > "),
> > + N_("git remote set-url [--both | --fetch | --push] --delete
> > "),
> > NULL
> > };
> >
> > @@ -1503,21 +1503,35 @@ static int set_branches(int
Re: [PATCH v3] remote: add --fetch and --both options to set-url
On Wed, Dec 17, 2014 at 03:20:58PM +0100, Peter Wu wrote:
> > There are 2 warning "dangling else", line 1570 and 1578
> > I think we should use:
> >
> > for (i = 0; i < remote->pushurl_nr; i++) {
> > if (!regexec(&old_regex, remote->pushurl[i], 0,
> > NULL, 0))
> > matches++;
> > else
> > negative_matches++;
> > }
> >
>
> Good catch, strange enough I did not get any warnings from my compiler
> or Clang's static analyzer. I have submitted a new patch which treats
> the accumulated comments.
Yeah, gcc does not seem to care, but compiling with clang (3.5.0 here)
does notice it.
C's parser matches what your indentation indicates, so it is just a
style fixup, not a bug (but one I agree is worth addressing).
-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 v4] remote: add --fetch and --both options to set-url
On Wed, Dec 17, 2014 at 03:18:56PM +0100, Peter Wu wrote: > This is the fourth revision of the patch that allows for just setting the > fetch > URL. Eric wondered why '--fetch --push' is not used to describe the state > '--both', so I added this to the commit message. Thanks, I think that explanation is very clear. This version overall looks good to me. > The t5505-remote.sh test still passes after this change (I was unable to run > the > full test suite as it broke due to an unrelated gpg issue). It is it because you have gpg 2.1, and the patches to handle that are not yet merged to master? Or is it because you are using the new patches and they are breaking things for your older gpg version? If the former, that's fine. If the latter, it would be nice to see a report of the breakage. :) -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 v4] remote: add --fetch and --both options to set-url
On Wednesday 17 December 2014 09:32:13 Jeff King wrote: > On Wed, Dec 17, 2014 at 03:18:56PM +0100, Peter Wu wrote: > > > This is the fourth revision of the patch that allows for just setting the > > fetch > > URL. Eric wondered why '--fetch --push' is not used to describe the state > > '--both', so I added this to the commit message. > > Thanks, I think that explanation is very clear. > > This version overall looks good to me. > > > The t5505-remote.sh test still passes after this change (I was unable to > > run the > > full test suite as it broke due to an unrelated gpg issue). > > It is it because you have gpg 2.1, and the patches to handle that are > not yet merged to master? Or is it because you are using the new patches > and they are breaking things for your older gpg version? > > If the former, that's fine. If the latter, it would be nice to see a > report of the breakage. :) Rest assured, this is a gpg 2.1 issue :-) -- Kind regards, Peter https://lekensteyn.nl -- 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
[PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes
From: Ronnie Sahlberg
Update receive-pack to use an atomic transaction iff the client negotiated
that it wanted atomic-push. This leaves the default behavior to be the old
non-atomic one ref at a time update. This is to cause as little disruption
as possible to existing clients. It is unknown if there are client scripts
that depend on the old non-atomic behavior so we make it opt-in for now.
If it turns out over time that there are no client scripts that depend on the
old behavior we can change git to default to use atomic pushes and instead
offer an opt-out argument for people that do not want atomic pushes.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Stefan Beller
---
Notes:
Changes v1 -> v2:
* update(...) assumes to be always in a transaction
* Caring about when to begin/commit transactions is put
into execute_commands
v2->v3:
* meditated about the error flow. Now we always construct a local
strbuf err if required. Then the flow is easier to follow and
destruction of it is performed nearby.
* early return in execute_commands if transaction_begin fails.
builtin/receive-pack.c | 83 +-
1 file changed, 68 insertions(+), 15 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e76e5d5..c942a3b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -67,6 +67,7 @@ static const char *NONCE_SLOP = "SLOP";
static const char *nonce_status;
static long nonce_stamp_slop;
static unsigned long nonce_stamp_slop_limit;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -823,6 +824,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
}
if (is_null_sha1(new_sha1)) {
+ struct strbuf err = STRBUF_INIT;
if (!parse_object(old_sha1)) {
old_sha1 = NULL;
if (ref_exists(name)) {
@@ -832,35 +834,36 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
cmd->did_not_exist = 1;
}
}
- if (delete_ref(namespaced_name, old_sha1, 0)) {
- rp_error("failed to delete %s", name);
+ if (ref_transaction_delete(transaction,
+ namespaced_name,
+ old_sha1,
+ 0, old_sha1 != NULL,
+ "push", &err)) {
+ rp_error("%s", err.buf);
+ strbuf_release(&err);
return "failed to delete";
}
+ strbuf_release(&err);
return NULL; /* good */
}
else {
struct strbuf err = STRBUF_INIT;
- struct ref_transaction *transaction;
-
if (shallow_update && si->shallow_ref[cmd->index] &&
update_shallow_ref(cmd, si))
return "shallow error";
- transaction = ref_transaction_begin(&err);
- if (!transaction ||
- ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, "push",
- &err) ||
- ref_transaction_commit(transaction, &err)) {
- ref_transaction_free(transaction);
-
+ if (ref_transaction_update(transaction,
+ namespaced_name,
+ new_sha1, old_sha1,
+ 0, 1, "push",
+ &err)) {
rp_error("%s", err.buf);
strbuf_release(&err);
+
return "failed to update ref";
}
-
- ref_transaction_free(transaction);
strbuf_release(&err);
+
return NULL; /* good */
}
}
@@ -1052,6 +1055,7 @@ static void execute_commands(struct command *commands,
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
+ struct strbuf err = STRBUF_INIT;
if (unpacker_error) {
for (cmd = commands; cmd; cmd = cmd->next)
@@ -1059,6 +1063,16 @@ static void execute_commands(struct command *commands,
return;
}
+ if (use_atomic) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ error("%s", err.buf);
+ strbuf_release(&err);
+ for (cmd = commands; cmd; cmd = cmd->next)
+ cmd->error_string = "transact
[PATCHv3 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes
This adds tests for the atomic push option.
The first four tests check if the atomic option works in
good conditions and the last three patches check if the atomic
option prevents any change to be pushed if just one ref cannot
be updated.
Signed-off-by: Stefan Beller
---
Notes:
Changes in v3:
* test_refs for checking
Changes v1 -> v2:
> Please drop unused comments; they are distracting.
ok
> It is not wrong per-se, but haven't you already tested in
> combination with --mirror in the previous test?
I fixed the previous tests, so that there is no --mirror
and --atomic together. There is still a first --mirror push
for setup and a second with --atomic though
> check_branches upstream master HEAD@{2} second HEAD~
A similar function test_ref_upstream is introduced.
> What's the value of this test? Isn't it a non-fast-forward check
> you already tested in the previous one?
I messed up there. Originally I wanted to test the 2 different
stages of rejection. A non-fast-forward check is done locally and
we don't even try pushing. But I also want to test if we locally
thing all is good, but the server refuses a ref to update.
This is now done with the last test named 'atomic push obeys
update hook preventing a branch to be pushed'. And that still fails.
I'll investigate that, while still sending out the series for another
review though.
* Redone the test helper, there is test_ref_upstream now.
This tests explicitely for SHA1 values of the ref.
(It's needed in the last test for example. The git push fails,
but still modifies the ref :/ )
* checked all && chains and repaired them
* sometimes make use of git -C
Notes v1:
Originally Ronnie had a similar patch prepared. But as I added
more tests and cleaned up the existing tests (e.g. use test_commit
instead of "echo one >file && gitadd file && git commit -a -m 'one'",
removal of dead code), the file has changed so much that I'd rather
take ownership.
t/t5543-atomic-push.sh | 178 +
1 file changed, 178 insertions(+)
create mode 100755 t/t5543-atomic-push.sh
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
new file mode 100755
index 000..b81a542
--- /dev/null
+++ b/t/t5543-atomic-push.sh
@@ -0,0 +1,178 @@
+#!/bin/sh
+
+test_description='pushing to a repository using the atomic push option'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf workbench upstream &&
+ test_create_repo upstream &&
+ test_create_repo workbench &&
+ (
+ cd upstream &&
+ git config receive.denyCurrentBranch warn
+ ) &&
+ (
+ cd workbench &&
+ git remote add up ../upstream
+ )
+}
+
+# Compare the ref ($1) in upstream with a ref value from workbench ($2)
+# i.e. test_refs second HEAD@{2}
+test_refs () {
+ test $# = 2 &&
+ git -C upstream rev-parse --verify "$1" >expect &&
+ git -C workbench rev-parse --verify "$2" >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'atomic push works for a single branch' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git push --mirror up &&
+ test_commit two &&
+ git push --atomic up master
+ ) &&
+ test_refs master master
+'
+
+test_expect_success 'atomic push works for two branches' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git branch second &&
+ git push --mirror up &&
+ test_commit two &&
+ git checkout second &&
+ test_commit three &&
+ git push --atomic up master second
+ ) &&
+ test_refs master master &&
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --mirror' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git checkout -b second &&
+ test_commit two &&
+ git push --atomic --mirror up
+ ) &&
+ test_refs master master &&
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --force' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git branch second master &&
+ test_commit two_a &&
+ git checkout second &&
+ test_commit two_b &&
+ test_commit three_b &&
+ test_commit four &&
+ git push --mirror up &&
+ # The actual test is below
+ git checkout master &&
+ test_commit three_a &&
+ git checkout second
[PATCHv3 3/6] send-pack.c: add --atomic command line argument
From: Ronnie Sahlberg
This adds support to send-pack to negotiate and use atomic pushes
iff the server supports it. Atomic pushes are activated by a new command
line flag --atomic.
In order to do this we also need to change the semantics for send_pack()
slightly. The existing send_pack() function actually doesn't send all the
refs back to the server when multiple refs are involved, for example
when using --all. Several of the failure modes for pushes can already be
detected locally in the send_pack client based on the information from the
initial server side list of all the refs as generated by receive-pack.
Any such refs that we thus know would fail to push are thus pruned from
the list of refs we send to the server to update.
For atomic pushes, we have to deal thus with both failures that are detected
locally as well as failures that are reported back from the server. In order
to do so we treat all local failures as push failures too.
We introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can
flag all refs that we would normally have tried to push to the server
but we did not due to local failures. This is to improve the error message
back to the end user to flag that "these refs failed to update since the
atomic push operation failed."
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Stefan Beller
---
Notes:
Notes:
Changes v1 -> v2:
* Now we only need a very small change in the existing code and have
a new static function, which cares about error reporting.
Junio wrote:
> Hmph. Is "atomic push" so special that it deserves a separate
> parameter? When we come up with yet another mode of failure,
would
> we add another parameter to the callers to this function?
* error messages are worded differently (lower case!),
* use of error function instead of fprintf
* undashed the printed error message ("atomic push failed");
Changes v2 -> v3:
> We avoid assignment inside a conditional.
Ok I switched to using a switch statement.
Documentation/git-send-pack.txt | 7 ++-
builtin/send-pack.c | 6 +-
remote.h| 3 ++-
send-pack.c | 40 ++--
transport.c | 4
5 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..45c7725 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another
repository
SYNOPSIS
[verse]
-'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=] [--verbose] [--thin] [:]
[...]
+'git send-pack' [--all] [--dry-run] [--force]
[--receive-pack=] [--verbose] [--thin] [--atomic]
[:] [...]
DESCRIPTION
---
@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush
packet.
Send a "thin" pack, which records objects in deltified form based
on objects not included in the pack to reduce network traffic.
+--atomic::
+ Use an atomic transaction for updating the refs. If any of the refs
+ fails to update then the entire push will fail without changing any
+ refs.
+
::
A remote host to house the repository. When this
part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..b961e5a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
#include "sha1-array.h"
static const char send_pack_usage[] =
-"git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=] [--verbose] [--thin] [:]
[...]\n"
+"git send-pack [--all | --mirror] [--dry-run] [--force]
[--receive-pack=] [--verbose] [--thin] [--atomic]
[:] [...]\n"
" --all and explicit specification are mutually exclusive.";
static struct send_pack_args args;
@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char
*prefix)
args.use_thin_pack = 1;
continue;
}
+ if (!strcmp(arg, "--atomic")) {
+ args.atomic = 1;
+ continue;
+ }
if (!strcmp(arg, "--stateless-rpc")) {
args.stateless_rpc = 1;
continue;
diff --git a/remote.h b/remote.h
index 8b62efd..f346524 100644
--- a/remote.h
+++ b/remote.h
@@ -115,7 +115,8 @@ struct ref {
REF_STATUS_REJECT_SHALLOW,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
- REF_STATUS_EXPECTING_REPORT
+ REF_STATUS_EXPECTING_REPORT,
+ REF_STATUS_ATOMIC_PUSH_FAILED
} stat
[PATCHv3 1/6] receive-pack.c: add protocol support to negotiate atomic
From: Ronnie Sahlberg
This adds support to the protocol between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Stefan Beller
---
Notes:
Changes v1 -> v2:
* Name it atomic instead of atomic-push. The name changes affects
the flags send over the wire as well as the flags in
struct send_pack_args
* Add check, which was part of the later patch here:
if (args->atomic && !atomic_supported) {
fprintf(stderr, "Server does not support atomic push.");
return -1;
}
v2 -> v3:
More concise error reporting as suggested by Junio
- fprintf(stderr, "Server does not support atomic push.");
- return -1;
+ return error("server does not support atomic push.");
Documentation/technical/protocol-capabilities.txt | 13 +++--
builtin/receive-pack.c| 6 +-
send-pack.c | 10 ++
send-pack.h | 3 ++-
4 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/Documentation/technical/protocol-capabilities.txt
b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..68ec23d 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client
requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress
server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic
+--
+
+If the server sends the 'atomic' capability it is capable of accepting
+atomic pushes. If the pushing client requests this capability, the server
+will update the refs in one single atomic transaction. Either all refs are
+updated or none.
+
allow-tip-sha1-in-want
--
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..e76e5d5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -171,7 +172,8 @@ static void show_ref(const char *path, const unsigned char
*sha1)
struct strbuf cap = STRBUF_INIT;
strbuf_addstr(&cap,
- "report-status delete-refs side-band-64k quiet");
+ "report-status delete-refs side-band-64k quiet "
+ "atomic");
if (prefer_ofs_delta)
strbuf_addstr(&cap, " ofs-delta");
if (push_cert_nonce)
@@ -1179,6 +1181,8 @@ static struct command *read_head_info(struct sha1_array
*shallow)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, "quiet"))
quiet = 1;
+ if (parse_feature_request(feature_list, "atomic"))
+ use_atomic = 1;
}
if (!strcmp(line, "push-cert")) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..e2bdd89 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int use_atomic;
+ int atomic_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,8 @@ int send_pack(struct send_pack_args *args,
agent_supported = 1;
if (server_supports("no-thin"))
args->use_thin_pack = 0;
+ if (server_supports("atomic"))
+ atomic_supported = 1;
if (args->push_cert) {
int len;
@@ -328,6 +3
[PATCHv3 5/6] push.c: add an --atomic argument
From: Ronnie Sahlberg
Add a command line argument to the git push command to request atomic
pushes.
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Stefan Beller
---
Notes:
Changes v1 -> v2
It's --atomic now! (dropping the -push)
v2->v3:
* s/atomic-push/atomic/
* s/the an/an/
* no serverside, but just remote instead
* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH
Documentation/git-push.txt | 7 ++-
builtin/push.c | 2 ++
transport.c| 1 +
transport.h| 1 +
4 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21b3f29..da63bdf 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
SYNOPSIS
[verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run]
[--receive-pack=]
+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n |
--dry-run] [--receive-pack=]
[--repo=] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream] [--signed]
[--force-with-lease[=[:]]]
@@ -136,6 +136,11 @@ already exists on the remote side.
logged. See linkgit:git-receive-pack[1] for the details
on the receiving end.
+--atomic::
+ Use an atomic transaction on the remote side if available.
+ Either all refs are updated, or on error, no refs are updated.
+ If the server does not support atomic pushes the push will fail.
+
--receive-pack=::
--exec=::
Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index a076b19..5731a0d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -518,6 +518,8 @@ int cmd_push(int argc, const char **argv, const char
*prefix)
OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant
tags"),
TRANSPORT_PUSH_FOLLOW_TAGS),
OPT_BIT(0, "signed", &flags, N_("GPG sign the push"),
TRANSPORT_PUSH_CERT),
+ OPT_BIT(0, "atomic", &flags, N_("use an atomic transaction
remote"),
+ TRANSPORT_PUSH_ATOMIC),
OPT_END()
};
diff --git a/transport.c b/transport.c
index c67feee..1373152 100644
--- a/transport.c
+++ b/transport.c
@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport,
struct ref *remote_re
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
args.push_cert = !!(flags & TRANSPORT_PUSH_CERT);
+ args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
args.url = transport->url;
ret = send_pack(&args, data->fd, data->conn, remote_refs,
diff --git a/transport.h b/transport.h
index 3e0091e..18d2cf8 100644
--- a/transport.h
+++ b/transport.h
@@ -125,6 +125,7 @@ struct transport {
#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
#define TRANSPORT_PUSH_CERT 2048
+#define TRANSPORT_PUSH_ATOMIC 4096
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) -
gettext_width(x)), (x)
--
2.2.0.31.gad78000.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 2/6] send-pack: Rename ref_update_to_be_sent to check_to_send_update
This renames ref_update_to_be_sent to check_to_send_update and inverts
the meaning of the return value. Having the return value inverted we
can have different values for the error codes. This is useful in a
later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED
case.
Signed-off-by: Stefan Beller
---
Notes:
This was introduced with the [PATCHv2] series.
Changes v2 -> v3:
* Rename to check_to_send_update
* Negative error values.
* errors values are #define'd and not raw numbers.
send-pack.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index e2bdd89..77e4201 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf
*sb)
for_each_commit_graft(advertise_shallow_grafts_cb, sb);
}
-static int ref_update_to_be_sent(const struct ref *ref, const struct
send_pack_args *args)
+#define CHECK_REF_NO_PUSH 1
+#define CHECK_REF_STATUS_REJECTED 2
+#define CHECK_REF_UPTODATE 3
+static int check_to_send_update(const struct ref *ref, const struct
send_pack_args *args)
{
if (!ref->peer_ref && !args->send_mirror)
- return 0;
+ return -CHECK_REF_NO_PUSH;
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
@@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref,
const struct send_pack_a
case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_REJECT_STALE:
case REF_STATUS_REJECT_NODELETE:
+ return -CHECK_REF_STATUS_REJECTED;
case REF_STATUS_UPTODATE:
- return 0;
+ return -CHECK_REF_UPTODATE;
default:
- return 1;
+ return 0;
}
}
@@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,
strbuf_addstr(&cert, "\n");
for (ref = remote_refs; ref; ref = ref->next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) < 0)
continue;
update_seen = 1;
strbuf_addf(&cert, "%s %s %s\n",
@@ -369,7 +373,7 @@ int send_pack(struct send_pack_args *args,
* the pack data.
*/
for (ref = remote_refs; ref; ref = ref->next) {
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) < 0)
continue;
if (!ref->deletion)
@@ -390,7 +394,7 @@ int send_pack(struct send_pack_args *args,
if (args->dry_run || args->push_cert)
continue;
- if (!ref_update_to_be_sent(ref, args))
+ if (check_to_send_update(ref, args) < 0)
continue;
old_hex = sha1_to_hex(ref->old_sha1);
--
2.2.0.31.gad78000.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 0/6] atomic pushes
This patch series adds a flag to git push to update the remote refs atomically. This series applies on top of origin/mh/reflog-expire It can also be found at github[2]. Changes v3->v3 are annotated in each patch. [2] https://github.com/stefanbeller/git/tree/atomic-push-v3 Ronnie Sahlberg (4): receive-pack.c: add protocol support to negotiate atomic-push send-pack.c: add --atomic command line argument receive-pack.c: use a single transaction when atomic-push is negotiated push.c: add an --atomic argument Stefan Beller (2): send-pack: Rename ref_update_to_be_sent to check_to_send_update t5543-atomic-push.sh: add basic tests for atomic pushes Documentation/git-push.txt| 7 +- Documentation/git-send-pack.txt | 7 +- Documentation/technical/protocol-capabilities.txt | 13 +- builtin/push.c| 2 + builtin/receive-pack.c| 89 +-- builtin/send-pack.c | 6 +- remote.h | 3 +- send-pack.c | 66 +++- send-pack.h | 3 +- t/t5543-atomic-push.sh| 178 ++ transport.c | 5 + transport.h | 1 + 12 files changed, 349 insertions(+), 31 deletions(-) create mode 100755 t/t5543-atomic-push.sh -- 2.2.0.31.gad78000.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Improving git branch
John Tapsell writes: > I'm interested in putting in some time and effort into improving the > output of "git branch". > > What I'm thinking is an output like this: > > $ git branch > > 2014-12-17 * (detached from origin/master) deaba04 Do stuff > 2014-12-15 john.ta/add_timing_info6edbcfa Add timing stuff > 2014-12-14 master 8537316 > [origin/master: ahead 1, behind 16] Some stuff > 2014-12-12 john.ta/new_reduce_memory 99d84db Reintroduce: memory stuff > 2014-12-05 john.ta/bugfixese15c95e Add stuff > 2014-12-03 john.ta/container e9fd4e5 This > branch is a test bed for containers > > > (These columns are supposed to be all aligned nicely..) > > So, features: > > 1. Show the date of the last commit > 2. Sort by date. Most recently used branches at the top > 3. Show the branch name, including your current "branch", with a * to > indicate that it's checked out. > 4. Show the sha > 5. Show the branch DESCRIPTION - and if that's not available, show the > short-line of the most recent commit. > > There is also a small amount of color here that I can't paste here, to > follow the coloring in the current git branch. > > Before I start making patches etc, what do people think? Would I have > a chance of getting this in? Should I change some aspects etc? Three random points: * A single output format can never be favourite of everybody, so this needs to be more like $ git branch --format='%(committerdate) %(refname) %(subject)' optionally with branch.format configuration variable to let the user specify the default. * I am not sure if the "current" marker should be anywhere but the frontmost column in the recommended default. The output from the command obviously is not meant for machine processing (e.g. sorting or grepping), so this point is minor, though. * I do not think the object name should take valuable screen real estate, again in the built-in default (I wouldn't mind people hurting themselves with their configuration at all ;-). After looking at "git branch --pretty-long" output, people can give any command john.ta/bugfixes instead of e15c95e, 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: Improving git branch
Michael J Gruber writes: > Rather than extending "git branch" any further[*], I suggest a bolder > strategy: > > - unify/merge for-each-ref and pretty formats (and code) as far as possible > - leverage that for the list modes of branch and tag > > That would allow everyone to get their favourite listing, just like for > logs. Otherwise it would be very difficult to agree on *the* proper > format for an extended branch or tag list. > > Michael > > > [*] I know I'm a sinner, too. Actually this is not a "bolder" strategy, but the unification has been discussed and agreed to be the longer-term direction for quite a while, I think. Didn't Peff have this in his "things to do when absolutely bored" box? -- 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: Improving git branch
On Wed, Dec 17, 2014 at 12:53:49PM -0800, Junio C Hamano wrote: > Michael J Gruber writes: > > > Rather than extending "git branch" any further[*], I suggest a bolder > > strategy: > > > > - unify/merge for-each-ref and pretty formats (and code) as far as possible > > - leverage that for the list modes of branch and tag > > > > That would allow everyone to get their favourite listing, just like for > > logs. Otherwise it would be very difficult to agree on *the* proper > > format for an extended branch or tag list. > > > > Michael > > > > > > [*] I know I'm a sinner, too. > > Actually this is not a "bolder" strategy, but the unification has > been discussed and agreed to be the longer-term direction for quite > a while, I think. Didn't Peff have this in his "things to do when > absolutely bored" box? Yes. It is not even in my "absolutely bored" box, but rather the "I would like to work on this but somehow other crap keeps coming up" box. The last blocker I ran into was that we need to unify the "--contains" implementation for "git tag" and "git branch". If anybody wants to push this forward, I think that is the best place to start. I can dig up references if anybody is interested. -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: Saving space/network on common repos
I am not sure if there was any improvement since then, but Junio wrote about alternates 2 years ago http://git-blame.blogspot.com/2012/08/bringing-bit-more-sanity-to-alternates.html -- 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
=-O
=-O -- 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 v4] remote: add --fetch and --both options to set-url
Peter Wu writes:
> git remote set-url knew about the '--push' option to update just the
> pushurl, but it does not have a similar option for "update fetch URL and
> leave whatever was in place for the push URL".
>
> This patch adds support for a '--fetch' option which implements that use
> case in a backwards compatible way: if no --both, --push or --fetch
> options are given, then the push URL is modified too if it was not set
> before. This is the case since the push URL is implicitly based on the
> fetch URL.
OK. In other words, for those without asymmetric configuration
without a need to define pushURL, this default should be the most
convenient, as it does not have to fiddle with two variables.
> A '--both' option is added to make the command independent of previous
> pushurl settings. For the --add and --delete set operations, it will
> always set the push and/ or the fetch URLs. For the primary mode of
"and/or", I think.
> operation (without --add or --delete), it will drop pushurl as the
> implicit push URL is the (fetch) URL.
I think this description is clear enough without "(if exists)" at
the end.
> While '--both' could be implemented as '--fetch --push', it might also
> be mistaken for "--push overrides --fetch". An option such as
> "--only={fetch|push|both}" was also considered, but it is longer than
> the current options, makes --push redundant and brings the confusing
> option "--only=both". Options such as '--direction=...' is too long and
> '--dir=' is ambiguous ("directory"?). Thus, for brevity three new
> options were introduced.
Sounds sensible.
> @@ -134,7 +134,15 @@ Changes URL remote points to. Sets first URL remote
> points to matching
> regex (first URL if no is given) to . If
> doesn't match any URL, error occurs and nothing is changed.
> +
> -With '--push', push URLs are manipulated instead of fetch URLs.
> +With '--both', both the fetch and push URLs are manipulated.
> ++
> +With '--fetch', only fetch URLs are manipulated.
> ++
> +With '--push', only push URLs are manipulated.
I am afraid that this part is confusing when read in the wider
context of the document.
The first sentence of this section (outside the patch) is "Changes
URL remote points to"; I expect that the most people would think
that it is talking about "remote.*.url" configuration, and the
wording for "--push" in the original is also clear that it touches
'remote.*.pushurl' instead.
In the updated text, you use words "fetch URL" and "push URL" to
mean "regardless of how these are represented by the configuration
system, the URL to be used for fetching/pushing". In other words,
in the updated vocabulary, 'remote.*.pushurl' is *NOT* called "push
URL" (and 'remote.*.url' is not 'fetch URL').
That may be easier for new people who aren't familiar with the
configuration system (read: I am saying that it may be a good idea
in the longer term), but the phrasing does not make it clear that
they are not referring remote.*.{url,pushURL} variables.
Rephrasing these to "URLs used for fetching" vs "URLs used for
pushing" may make things a bit less confusion-prone.
In any case, we would also need to update Documentation/config.txt
which has these:
remote..url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
remote..pushurl::
The push URL of a remote repository. See linkgit:git-push[1].
It may be sufficient to rephrase them like so:
remote..url::
The URL of a remote repository, used for fetching from it
and pushing into it unless a separate remote..pushURL
is defined. See linkgit:git-fetch[1] and linkgit:git-push[1]
remote..pushurl::
The URL of a remote repository, used for pushing into it
(if undefined, remote..url is used to push into it).
See linkgit:git-push[1].
perhaps?
> +For historical reasons, if neither --fetch nor --push is specified then the
> +fetch URL is changed, as well as the push URL if this was not already set.
> This
> +behavior may change in the future.
This paragraph is unwarranted. As you explained in the second
paragraph in the log message, the traditional behaviour was a good
default for majority of people and I do not think we saw any
demonstrated need to deprecate it.
> +#define MODIFY_TYPE_FETCH (1 << 0)
> +#define MODIFY_TYPE_PUSH(1 << 1)
> +#define MODIFY_TYPE_BOTH(MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH)
> +/* The historic behavior behaves like --fetch, but does not touch the push
> URL
> + * configuration (and thereby may appear to change the push URL too if this
> was
> + * not set before).
> + */
> +#define MODIFY_TYPE_HISTORIC(MODIFY_TYPE_FETCH | (1 << 2))
/*
* Our multi-line comment begins with a slash-asterisk
* without anything else on the remainder of the line
* and ends with an asterisk-slash, possibly indented
* and nothing else on it.
*/
Per
Re: Saving space/network on common repos
(+cc: Duy who wrote the recent 'checkout --to' patch series)
Hi Craig,
Craig Silverstein wrote:
> By design, our Jenkins machine has several different
> directories that each hold a copy of the same git repository. (For
> instance, Jenkins may be running tests on our repo at several
> different commits at the same time.) When Jenkins decides to run a
> test -- I'm simplifying a bit -- it will pick one of the copies of the
> repo, do a 'git fetch origin && git checkout ' and the
> run the tests.
You might find 'git new-workdir' from contrib/workdir to be helpful.
It lets you attach multiple working copies to a single set of objects
and refs.
There's a patch series to move that functionality into core git
through an option "git checkout --to=" that creates a
new workdir for an existing repository that is currently in the
pu ("proposed updates") branch.
[...]
> An added complication is submodules. We have a submodule that is as
> big and slow to fetch as our main repository.
>
> Question 4) Is there a practical way to set up submodules so they can
> use the same object-sharing framework that the main repo does?
It's possible to do, but we haven't written a nice UI for it yet.
(In other words, you can do this by cloning with --no-recurse-submodules
and manually creating the submodule workdir in the appropriate place.
Later calls to "git submodule update" will do the right thing.)
Thanks for a useful example,
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] git-compat-util: suppress unavoidable Apple-specific deprecation warnings
Eric Sunshine writes: > These Apple-specific warnings are pure noise: they don't tell us > anything useful and we have no control over them, nor is Apple likely to > provide replacements any time soon. Such noise may obscure other > legitimate warnings, therefore silence them. Sad but it appears to me that it is the best we could do... Will queue. Thanks. > > Signed-off-by: Eric Sunshine > --- > > Related discussion: > http://thread.gmane.org/gmane.comp.version-control.git/260463/ > > git-compat-util.h | 4 > 1 file changed, 4 insertions(+) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 400e921..433b8f2 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -211,8 +211,12 @@ extern char *gitbasename(char *); > #endif > > #ifndef NO_OPENSSL > +#define __AVAILABILITY_MACROS_USES_AVAILABILITY 0 > +#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6 > #include > #include > +#undef MAC_OS_X_VERSION_MIN_REQUIRED > +#undef __AVAILABILITY_MACROS_USES_AVAILABILITY > #endif > > /* On most systems would have given us this, but -- 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: [PATCHv3 2/6] send-pack: Rename ref_update_to_be_sent to check_to_send_update
Stefan Beller writes:
> This renames ref_update_to_be_sent to check_to_send_update and inverts
> the meaning of the return value. Having the return value inverted we
> can have different values for the error codes. This is useful in a
> later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED
> case.
Makes sense; I would have defined these constants negative, but that
can be cleaned up if somebody cares deeply enough later.
Will queue. Thanks.
>
> Signed-off-by: Stefan Beller
> ---
>
> Notes:
> This was introduced with the [PATCHv2] series.
> Changes v2 -> v3:
>
> * Rename to check_to_send_update
> * Negative error values.
> * errors values are #define'd and not raw numbers.
>
> send-pack.c | 18 +++---
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index e2bdd89..77e4201 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -190,10 +190,13 @@ static void advertise_shallow_grafts_buf(struct strbuf
> *sb)
> for_each_commit_graft(advertise_shallow_grafts_cb, sb);
> }
>
> -static int ref_update_to_be_sent(const struct ref *ref, const struct
> send_pack_args *args)
> +#define CHECK_REF_NO_PUSH 1
> +#define CHECK_REF_STATUS_REJECTED 2
> +#define CHECK_REF_UPTODATE 3
> +static int check_to_send_update(const struct ref *ref, const struct
> send_pack_args *args)
> {
> if (!ref->peer_ref && !args->send_mirror)
> - return 0;
> + return -CHECK_REF_NO_PUSH;
>
> /* Check for statuses set by set_ref_status_for_push() */
> switch (ref->status) {
> @@ -203,10 +206,11 @@ static int ref_update_to_be_sent(const struct ref *ref,
> const struct send_pack_a
> case REF_STATUS_REJECT_NEEDS_FORCE:
> case REF_STATUS_REJECT_STALE:
> case REF_STATUS_REJECT_NODELETE:
> + return -CHECK_REF_STATUS_REJECTED;
> case REF_STATUS_UPTODATE:
> - return 0;
> + return -CHECK_REF_UPTODATE;
> default:
> - return 1;
> + return 0;
> }
> }
>
> @@ -250,7 +254,7 @@ static int generate_push_cert(struct strbuf *req_buf,
> strbuf_addstr(&cert, "\n");
>
> for (ref = remote_refs; ref; ref = ref->next) {
> - if (!ref_update_to_be_sent(ref, args))
> + if (check_to_send_update(ref, args) < 0)
> continue;
> update_seen = 1;
> strbuf_addf(&cert, "%s %s %s\n",
> @@ -369,7 +373,7 @@ int send_pack(struct send_pack_args *args,
>* the pack data.
>*/
> for (ref = remote_refs; ref; ref = ref->next) {
> - if (!ref_update_to_be_sent(ref, args))
> + if (check_to_send_update(ref, args) < 0)
> continue;
>
> if (!ref->deletion)
> @@ -390,7 +394,7 @@ int send_pack(struct send_pack_args *args,
> if (args->dry_run || args->push_cert)
> continue;
>
> - if (!ref_update_to_be_sent(ref, args))
> + if (check_to_send_update(ref, args) < 0)
> continue;
>
> old_hex = sha1_to_hex(ref->old_sha1);
--
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: [PATCHv3 3/6] send-pack.c: add --atomic command line argument
Stefan Beller writes: > From: Ronnie Sahlberg > > This adds support to send-pack to negotiate and use atomic pushes > iff the server supports it. Atomic pushes are activated by a new command > line flag --atomic. Looks sensible. 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
Introducing a test_create_repo_bare (was Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes)
On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote: >> + ( >> + cd upstream && git config receive.denyCurrentBranch warn >> + ) && > > I was wondering how you would do this part after suggesting use of > test_create_repo, knowing very well that one of them was a bare one > ;-). > > We might want to extend test_create_repo to allow creating a bare > repository, but this is also OK. So I searching through all the tests, where it would make sense to do that. I searched for "denyCurrentBranch" and came up with this list where I think it makes sense to replace (git init | test_create_repo | or alike) by a test_create_repo_bare or add the --bare option to test_create_repo places where test_create_repo_bare is easily introducable: t5517-push-mirror.sh # setup an upstream repo t5543-atomic-push.sh # setup an upstream repo t5701-clone-local.sh # Test 'clone empty repository' not as easy: t5400-send-pack.sh # we commit to that repo while being inside t5405-send-pack-rewind.sh # we commit to that repo while being inside t5516-fetch-push.sh # we test the various denyCurrentBranch options unsure: t5522-pull-symlink.sh # just cloning the repo So I think we don't need the test_create_repo_bare yet. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes
Stefan Beller writes:
> @@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,
>
> if (cmd->skip_update)
> continue;
> -
> + if (!use_atomic) {
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + rp_error("%s", err.buf);
> + strbuf_release(&err);
> + cmd->error_string = "failed to start
> transaction";
> + return;
> + }
> + }
> cmd->error_string = update(cmd, si);
> + if (!use_atomic)
> + if (ref_transaction_commit(transaction, &err)) {
> + ref_transaction_free(transaction);
> + rp_error("%s", err.buf);
> + strbuf_release(&err);
> + cmd->error_string = "failed to update ref";
> + return;
> + }
Hmm, should the code even attempt to commit if update() returned a
non NULL, signaling a failure?
Or would we want to do this instead?
if (cmd->error_string)
goto transaction_abort;
else if (!use_atomic) {
if (ref_transaction_commit(...)) {
...
cmd->error_string = "...";
return;
}
}
and then ...
> if (shallow_update && !cmd->error_string &&
> si->shallow_ref[cmd->index]) {
> error("BUG: connectivity check has not been run on ref
> %s",
> @@ -1096,10 +1127,32 @@ static void execute_commands(struct command *commands,
> }
> }
>
> + if (use_atomic) {
> + /*
> + * update(...) may abort early (i.e. because the hook refused to
> + * update that ref) which then doesn't even record a transaction
> + * regarding that ref. Make sure all commands are without error
> + * and then commit atomically.
> + */
> + for (cmd = commands; cmd; cmd = cmd->next)
> + if (cmd->error_string)
> + break;
> + if (cmd) {
> + for (cmd = commands; cmd; cmd = cmd->next)
> + if (!cmd->error_string)
> + cmd->error_string = "atomic push
> failure";
> + } else if (ref_transaction_commit(transaction, &err)) {
> + rp_error("%s", err.buf);
> + for (cmd = commands; cmd; cmd = cmd->next)
> + cmd->error_string = err.buf;
> + }
... have the label to jump to here:
transaction_abort:
> + ref_transaction_free(transaction);
I was confused by the fact that you did not have any call to
transaction-abort, until I realized that there is no such API
function and ref_transaction_free() serves that "don't commit,
roll it back" purpose.
> + }
> if (shallow_update && !checked_connectivity)
> error("BUG: run 'git fsck' for safety.\n"
> "If there are errors, try to remove "
> "the reported refs above");
> + strbuf_release(&err);
> }
>
> static struct command **queue_command(struct command **tail,
--
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: Introducing a test_create_repo_bare (was Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes)
Stefan Beller writes: > On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote: > >>> + ( >>> + cd upstream && git config receive.denyCurrentBranch warn >>> + ) && >> >> I was wondering how you would do this part after suggesting use of >> test_create_repo, knowing very well that one of them was a bare one >> ;-). >> >> We might want to extend test_create_repo to allow creating a bare >> repository, but this is also OK. > > So I searching through all the tests, where it would make sense to do that. > I searched for "denyCurrentBranch" and came up with this list where I think > it makes sense to replace (git init | test_create_repo | or alike) by a > test_create_repo_bare or add the --bare option to test_create_repo > > places where test_create_repo_bare is easily introducable: > t5517-push-mirror.sh # setup an upstream repo > t5543-atomic-push.sh # setup an upstream repo > t5701-clone-local.sh # Test 'clone empty repository' > > not as easy: > t5400-send-pack.sh # we commit to that repo while being inside > t5405-send-pack-rewind.sh # we commit to that repo while being inside > t5516-fetch-push.sh # we test the various denyCurrentBranch options > > unsure: > t5522-pull-symlink.sh # just cloning the repo > > So I think we don't need the test_create_repo_bare yet. Thanks for digging. We already knew we do not *NEED* it. We have been surviving without one. You need to remember that adding and using a new helper is *NOT* the ultimate goal; categorizing those that do not want bare repositories as "not as easy" is misguided. They truly do not want bare, so they are not our target audience in the first place. For the same reason, "easily introduceable" is not a good criteria to look for. The issue is if some existing tests will be helped, if we had such a helper. That is, do we "git init --bare" by hand in some test? Are these tests in such a hand-crafted repositories more susceptible to future breakages because they do not use the template from the built location or they do not disable hooks? If we had such tests, then they would benefit by having a "bare" mode of test_create_repo. -- 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: Saving space/network on common repos
On Wed, Dec 17, 2014 at 2:32 PM, Jonathan Nieder wrote: > You might find 'git new-workdir' from contrib/workdir to be helpful. > It lets you attach multiple working copies to a single set of objects > and refs. Thanks! That does indeed sound promising -- like a more principled version of my GIT_OBJECT_DIRECTORY suggestion. >> Question 4) Is there a practical way to set up submodules so they can >> use the same object-sharing framework that the main repo does? > > It's possible to do, but we haven't written a nice UI for it yet. > (In other words, you can do this by cloning with --no-recurse-submodules > and manually creating the submodule workdir in the appropriate place. Hmm, let me see if I understand you right -- you're suggesting that when cloning my reference repo, I do git clone --no-recurse-submodules for (path, url) in `parse-.gitmodules`: git clone url path # this is psuedocode, obviously :-) and then when I want to create a new workdir, I do something like: cd reference_repo git new-workdir /var/workspace1 for (path, url) in `parse-.gitmodules`: cd path && git new-workdir /var/workspace1/path ? Basically, I'm going back to the old git way of having each submodule have its own .git directory, rather than having it have a .git file with a 'gitdir' entry. Am I understanding this right? Also, it seems to me there's the possibility, with git-newdir, that if several of the workspaces try to fetch at the same time they could step on each others' toes. Is that a problem? I know there's a push lock but I don't believe there's a fetch lock, and I could imagine git getting unhappy if two fetches happened in the same repo at the same time. craig -- 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: [PATCHv3 4/6] receive-pack.c: use a single ref_transaction for atomic pushes
On Wed, Dec 17, 2014 at 3:26 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> @@ -1086,8 +1100,25 @@ static void execute_commands(struct command *commands,
>>
>> if (cmd->skip_update)
>> continue;
>> -
>> + if (!use_atomic) {
>> + transaction = ref_transaction_begin(&err);
>> + if (!transaction) {
>> + rp_error("%s", err.buf);
>> + strbuf_release(&err);
>> + cmd->error_string = "failed to start
>> transaction";
>> + return;
>> + }
>> + }
>> cmd->error_string = update(cmd, si);
>> + if (!use_atomic)
>> + if (ref_transaction_commit(transaction, &err)) {
>> + ref_transaction_free(transaction);
>> + rp_error("%s", err.buf);
>> + strbuf_release(&err);
>> + cmd->error_string = "failed to update ref";
>> + return;
>> + }
>
> Hmm, should the code even attempt to commit if update() returned a
> non NULL, signaling a failure?
>
> Or would we want to do this instead?
This would change the current behavior. In the case of !atomic we want
to consider all commands and not stop early.
So maybe more
if (!cmd->error_string) {
if (!use_atomic
&& ref_transaction_commit(...)) {
...
}
} else {
if (use_atomic)
goto check_atomic_commit;
}
and the check_atomic_commit label is replacing the loop to check:
-for (cmd = commands; cmd; cmd = cmd->next)
-if (cmd->error_string)
-break;
+ check_atomic_commit:
--
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: Saving space/network on common repos
Craig Silverstein wrote: > On Wed, Dec 17, 2014 at 2:32 PM, Jonathan Nieder wrote: >> Craig Silverstein wrote: >>> Question 4) Is there a practical way to set up submodules so they can >>> use the same object-sharing framework that the main repo does? >> >> It's possible to do, but we haven't written a nice UI for it yet. >> (In other words, you can do this by cloning with --no-recurse-submodules >> and manually creating the submodule workdir in the appropriate place. > > Hmm, let me see if I understand you right -- you're suggesting that > when cloning my reference repo, I do > git clone --no-recurse-submodules > for (path, url) in `parse-.gitmodules`: git clone url path > # this is psuedocode, obviously :-) > > and then when I want to create a new workdir, I do something like: > cd reference_repo > git new-workdir /var/workspace1 > for (path, url) in `parse-.gitmodules`: cd path && git new-workdir > /var/workspace1/path > > ? Basically, I'm going back to the old git way of having each > submodule have its own .git directory, rather than having it have a > .git file with a 'gitdir' entry. Am I understanding this right? Basically. The initial clone can still use --recurse-submodules. When you create a new workdir you'd create new workdirs for the submodules by hand. A 'git submodule foreach' command in the initial repo can take care of the `parse-.gitmodules` part. [...] > Also, it seems to me there's the possibility, with git-newdir, that if > several of the workspaces try to fetch at the same time they could > step on each others' toes. Is that a problem? I know there's a push > lock but I don't believe there's a fetch lock, and I could imagine git > getting unhappy if two fetches happened in the same repo at the same > time. That's a good question. If concurrent fetches cause trouble then I'd consider it a bug (it's not too different from multiple concurrent pushes to the same repository, which is a very common thing to do), but I haven't looked carefully into whether such bugs exist. Hopefully someone else can chime in. 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: Introducing a test_create_repo_bare (was Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes)
On Wed, Dec 17, 2014 at 3:51 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Tue, Dec 16, 2014 at 11:46 AM, Junio C Hamano wrote: >> + ( + cd upstream && git config receive.denyCurrentBranch warn + ) && >>> >>> I was wondering how you would do this part after suggesting use of >>> test_create_repo, knowing very well that one of them was a bare one >>> ;-). >>> >>> We might want to extend test_create_repo to allow creating a bare >>> repository, but this is also OK. >> >> So I searching through all the tests, where it would make sense to do that. >> I searched for "denyCurrentBranch" and came up with this list where I think >> it makes sense to replace (git init | test_create_repo | or alike) by a >> test_create_repo_bare or add the --bare option to test_create_repo >> >> places where test_create_repo_bare is easily introducable: >> t5517-push-mirror.sh # setup an upstream repo >> t5543-atomic-push.sh # setup an upstream repo >> t5701-clone-local.sh # Test 'clone empty repository' >> >> not as easy: >> t5400-send-pack.sh # we commit to that repo while being inside >> t5405-send-pack-rewind.sh # we commit to that repo while being inside >> t5516-fetch-push.sh # we test the various denyCurrentBranch options >> >> unsure: >> t5522-pull-symlink.sh # just cloning the repo >> >> So I think we don't need the test_create_repo_bare yet. > > Thanks for digging. > > We already knew we do not *NEED* it. We have been surviving without > one. > > You need to remember that adding and using a new helper is *NOT* the > ultimate goal; categorizing those that do not want bare repositories > as "not as easy" is misguided. They truly do not want bare, so they > are not our target audience in the first place. For the same reason, > "easily introduceable" is not a good criteria to look for. > > The issue is if some existing tests will be helped, if we had such a > helper. That is, do we "git init --bare" by hand in some test? Are > these tests in such a hand-crafted repositories more susceptible to > future breakages because they do not use the template from the built > location or they do not disable hooks? If we had such tests, then > they would benefit by having a "bare" mode of test_create_repo. > > You're right, I was looking at the wrong criteria. There are rougly 125 " --bare" outside outside of the t0001-init.sh and t5601-clone.sh which are not relying on the template nor the $GIT_EXEC_PATH/git-init The occurrences outside these two tests are mostly init and clone with --bare although we also have a few "git --bare foo" commands to operate on the bare repos. Looking at it the other way round it seems to be more work to fix it. :/ -- 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: Introducing a test_create_repo_bare (was Re: [PATCHv2 6/6] t5543-atomic-push.sh: add basic tests for atomic pushes)
Junio C Hamano wrote: > The issue is if some existing tests will be helped, if we had such a > helper. Since both bin-wrappers/git and test-lib.sh set GIT_TEMPLATE_DIR and templates/blt doesn't contain any enabled hooks, I don't see how such a helper would be useful. If making things more consistent were worth the churn, then if anything it would make sense to make test_create_repo private to the setup code in test-lib.sh and to use plain 'git init ' in tests. 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
[PATCH 1/2] Documentation/SubmittingPatches: Explain the rationale of git notes
This adds an explanation of why you want to have the --notes option given to git format-patch. Signed-off-by: Stefan Beller --- Notes: > with optionally update Documentation/SubmittingPatches > to point at the file. That file actually talks about notes already. I am sending two patches touching that file, one of them explaining the --notes workflow rationale and one of them just changing white spaces. A few weeks ago I wanted to patch format-patch to remove change ids. This is not needed any more for the git workflow as I disabled them and do not upload any patches to an optional Gerrit code review server anymore. I do like the workflow using --notes as well from a developers perspective as I take literally notes for my own sanity. I wonder if I should add a config format.notes = [default-off, always, on-if-non-empty] so I don't need always add --notes manually to the command line. Thanks, Stefan Documentation/SubmittingPatches | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index fa71b5f..16b5d65 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -176,7 +176,11 @@ message starts, you can put a "From: " line to name that person. You often want to add additional explanation about the patch, other than the commit message itself. Place such "cover letter" material between the three dash lines and the diffstat. Git-notes -can also be inserted using the `--notes` option. +can also be inserted using the `--notes` option. If you are one +of those developers who cannot write perfect code the first time +and need multiple iterations of review and discussion, you are +encouraged to use the notes to describe the changes between the +different versions of a patch. Do not attach the patch as a MIME attachment, compressed or not. Do not let your e-mail client send quoted-printable. Do not let -- 2.2.0.31.gad78000.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Documentation/SubmittingPatches: unify whitespace/tabs for the DCO
The Developers Certificate of Origin has a mixture of tabs and white spaces which is annoying to view if your editor explicitly views white space characters. Signed-off-by: Stefan Beller --- Notes: I'll also try to send it upstream to linux. Documentation/SubmittingPatches | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 16b5d65..6124f34 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -258,15 +258,15 @@ pretty simple: if you can certify the below: person who certified (a), (b) or (c) and I have not modified it. - (d) I understand and agree that this project and the contribution - are public and that a record of the contribution (including all - personal information I submit with it, including my sign-off) is - maintained indefinitely and may be redistributed consistent with - this project or the open source license(s) involved. +(d) I understand and agree that this project and the contribution +are public and that a record of the contribution (including all +personal information I submit with it, including my sign-off) is +maintained indefinitely and may be redistributed consistent with +this project or the open source license(s) involved. then you just add a line saying - Signed-off-by: Random J Developer +Signed-off-by: Random J Developer This line can be automatically added by Git if you run the git-commit command with the -s option. -- 2.2.0.31.gad78000.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Documentation/SubmittingPatches: unify whitespace/tabs for the DCO
The Developers Certificate of Origin has a mixture of tabs and white spaces which is annoying to view if your editor explicitly views white space characters. Signed-off-by: Stefan Beller --- Documentation/SubmittingPatches | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 16b5d65..6124f34 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -258,15 +258,15 @@ pretty simple: if you can certify the below: person who certified (a), (b) or (c) and I have not modified it. - (d) I understand and agree that this project and the contribution - are public and that a record of the contribution (including all - personal information I submit with it, including my sign-off) is - maintained indefinitely and may be redistributed consistent with - this project or the open source license(s) involved. +(d) I understand and agree that this project and the contribution +are public and that a record of the contribution (including all +personal information I submit with it, including my sign-off) is +maintained indefinitely and may be redistributed consistent with +this project or the open source license(s) involved. then you just add a line saying - Signed-off-by: Random J Developer +Signed-off-by: Random J Developer This line can be automatically added by Git if you run the git-commit command with the -s option. -- 2.2.0.31.gad78000.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug & patch: exit codes from internal commands are handled incorrectly
The situation is actually slightly more complex than I stated previously. From
the docs:
The exit value of this program is negative on error,
But there’s no such thing as a negative error code under Unix, so (at best)
that will be exit(255).
No patch, because this is getting painfully close to needing someone with a
global view of the code to fix.
Thanks,
Keni
On Dec 16, 2014, at 2:28 PM, Kenneth Lorber wrote:
> (Apologies if I’ve missed anything here - I’m still climbing the git learning
> curve.)
>
> Bug: exit codes from (at least) internal commands are handled incorrectly.
> E.g. git-merge-file, docs say:
>The exit value of this program is negative on error, and the number of
>conflicts otherwise. If the merge was clean, the exit value is 0.
> But only 8 bits get carried through exit, so 256 conflicts gives exit(0),
> which means the merge was clean.
>
> Issue shows up on:
> git version 1.8.5.2 (Apple Git-48)
> build from source (git version 2.2.0.68.g64396d6.dirty after patch below
> applied)
>
> Reproduce by:
>
> Put the following test program in an empty directory as buggen.pl
>
> TEST PROGRAM START
> open OUTB, ">basefile" or die;
> print OUTB "this is the base file\n";
> close OUTB;
>
> open OUT1, ">bfile1" or die;
> open OUT2, ">bfile2" or die;
>
> $c = shift;
>
> while($c > 0){
>print OUT1 "a\nb\nc\nd\nchange $c file 1\n";
>print OUT2 "a\nb\nc\nd\nchange $c file 2\n";
>$c--;
> }
> TEST PROGRAM END
>
> Do these tests:
>
> perl buggen.pl 0
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 0
>
> perl buggen.pl 1
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 1
>
> perl buggen.pl 256
> git merge-file -p bfile1 basefile bfile2
> echo $status
> 0 <===OOPS
>
> Proposed patches:
> diff --git a/git.c b/git.c
> index 82d7a1c..8228a3c 100644
> --- a/git.c
> +++ b/git.c
> @@ -349,6 +349,8 @@ static int run_builtin(struct cmd_struct *p, int argc,
> const
>trace_argv_printf(argv, "trace: built-in: git");
>
>status = p->fn(argc, argv, prefix);
> + if (status > 255)
> + status = 255; /* prevent exit() from truncating to 0 */
>if (status)
>return status;
> diff --git a/Documentation/git-merge-file.txt
> b/Documentation/git-merge-file.txt
> index d2fc12e..76e6a11 100644
> --- a/Documentation/git-merge-file.txt
> +++ b/Documentation/git-merge-file.txt
> @@ -41,7 +41,8 @@ lines from ``, or lines from both respectively.
> T
> conflict markers can be given with the `--marker-size` option.
>
> The exit value of this program is negative on error, and the number of
> -conflicts otherwise. If the merge was clean, the exit value is 0.
> +conflicts otherwise (but 255 will be reported even if more than 255 conflicts
> +exist). If the merge was clean, the exit value is 0.
>
> 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it
> implements all of RCS 'merge''s functionality which is needed by
>
> Open questions:
> 1) Is 255 safe for all operating systems?
> 2) Does this issue affect any other places?
>
> Thanks,
> Keni
> k...@his.com
>
--
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
