[GSoC][PATCH v2 0/1] rebase -i: rewrite append_todo_help() in C
This patch rewrites append_todo_help() from shell to C. The C version covers a bit more than the old shell version. To achieve that, some parameters were added to rebase--helper. This is part of the effort to rewrite interactive rebase in C. Changes since v1: - Renaming the parameter to insert the edit-todo message from `edit-todo` to `write-edit-todo`. - Clarifying the `write-edit-todo` help message. - Dropping the commit that removed newlines in the messages. Alban Gruin (1): rebase--interactive: rewrite append_todo_help() in C builtin/rebase--helper.c | 10 ++-- git-rebase--interactive.sh | 52 ++-- sequencer.c| 60 ++ sequencer.h| 1 + 4 files changed, 71 insertions(+), 52 deletions(-) -- 2.16.4
[GSoC][PATCH v2 1/1] rebase--interactive: rewrite append_todo_help() in C
This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.
Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--write-edit-todo`).
Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.
Signed-off-by: Alban Gruin
---
builtin/rebase--helper.c | 10 ++--
git-rebase--interactive.sh | 52 ++--
sequencer.c| 60 ++
sequencer.h| 1 +
4 files changed, 71 insertions(+), 52 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc..7ef92fbb6 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, edit_todo = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC
+ ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +27,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
N_("keep original branch points of cousins")),
+ OPT_BOOL(0, "write-edit-todo", _todo,
+N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -45,6 +47,8 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
+ OPT_CMDMODE(0, "append-todo-help", ,
+ N_("insert the help in the todo list"),
APPEND_TODO_HELP),
OPT_END()
};
@@ -84,5 +88,7 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+ if (command == APPEND_TODO_HELP && argc == 1)
+ return !!append_todo_help(edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded213..94c23a7af 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
}
-append_todo_help () {
- gettext "
-Commands:
-p, pick = use commit
-r, reword = use commit, but edit the commit message
-e, edit = use commit, but stop for amending
-s, squash = use commit, but meld into previous commit
-f, fixup = like \"squash\", but discard this commit's log message
-x, exec = run command (the rest of the line) using shell
-d, drop = remove commit
-l, label = label current HEAD with a name
-t, reset = reset HEAD to a label
-m, merge [-C | -c ] [# ]
-. create a merge commit using the original merge commit's
-. message (or the oneline, if no original merge commit was
-. specified). Use -c to reword the commit message.
-
-These lines can be re-ordered; they are executed from top to bottom.
-" | git stripspace --comment-lines >>"$todo"
-
- if test $(get_missing_commit_check_level) = error
- then
- gettext "
-Do not remove any line. Use 'drop' explicitly to remove a commit.
-" | git stripspace --comment-lines >>"$todo"
- else
- gettext "
-If you remove a line here THAT COMMIT WILL BE LOST.
-" | git stripspace --comment-lines >>"$todo"
- fi
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -143,13 +111,7 @@ initiate_action () {
git stripspace --strip-comments <"$todo" >"$todo".new
mv -f "$todo".new "$todo"
collapse_todo_ids
- append_todo_help
- gettext "
-You are
Re: Git not creating merge commit when merging signed/annotated tag
On Tue, Jun 05 2018, Tim Friske wrote: > Hi Everyone, > > ten days ago I asked on https://unix.stackexchange.com/ why Git is not > creating a merge commit when merging a signed/annotated tag. Does someone has > an answer to my question > https://unix.stackexchange.com/questions/446154/git-not-creating-merge-commit-when-merging-signed-annotated-tag? > > Thank you in advance! I believe my answer in this recent thread which brought up the same topic applies to your situation as well: https://public-inbox.org/git/CANgJU+VFCY0LNRLMSGtD7ScpcLaPFMzUOyw6Bjgk6q=kx9d...@mail.gmail.com/ Try to apply the patch I noted to builtin/merge.c there and see if you don't get the same message printed out.
Git not creating merge commit when merging signed/annotated tag
Hi Everyone, ten days ago I asked on https://unix.stackexchange.com/ why Git is not creating a merge commit when merging a signed/annotated tag. Does someone has an answer to my question https://unix.stackexchange.com/questions/446154/git-not-creating-merge-commit-when-merging-signed-annotated-tag? Thank you in advance! – Tim
Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test
On Tue, 5 Jun 2018, SZEDER Gábor wrote: > > Change deprecated "--set-upstream" branch option to > > preferred "--set-upstream-to". > > > > Signed-off-by: Robert P. J. Day > > > > --- > > > > i'm assuming this should use "--set-upstream-to" as do all the > > others. > > I don't think so, see 52668846ea (builtin/branch: stop supporting > the "--set-upstream" option, 2017-08-17). yes, you're right, i didn't look at the context enough. my bad. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test
> Change deprecated "--set-upstream" branch option to > preferred "--set-upstream-to". > > Signed-off-by: Robert P. J. Day > > --- > > i'm assuming this should use "--set-upstream-to" as do all the > others. I don't think so, see 52668846ea (builtin/branch: stop supporting the "--set-upstream" option, 2017-08-17). Though arguably the test name could be more descriptive and tell why it should fail. > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 69ea8202f4..ef887a0b32 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -885,8 +885,8 @@ test_expect_success 'test --unset-upstream on a > particular branch' ' > test_must_fail git config branch.my14.merge > ' > > -test_expect_success '--set-upstream fails' ' > -test_must_fail git branch --set-upstream origin/master > +test_expect_success '--set-upstream-to fails' ' > +test_must_fail git branch --set-upstream-to origin/master > ' > > test_expect_success '--set-upstream-to notices an error to set branch as own > upstream' ' > > -- > > > Robert P. J. Day Ottawa, Ontario, CANADA > http://crashcourse.ca/dokuwiki > > Twitter: http://twitter.com/rpjday > LinkedIn: http://ca.linkedin.com/in/rpjday > >
[PATCH] t3200-branch.sh: use "--set-upstream-to" in test
Change deprecated "--set-upstream" branch option to preferred "--set-upstream-to". Signed-off-by: Robert P. J. Day --- i'm assuming this should use "--set-upstream-to" as do all the others. diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 69ea8202f4..ef887a0b32 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -885,8 +885,8 @@ test_expect_success 'test --unset-upstream on a particular branch' ' test_must_fail git config branch.my14.merge ' -test_expect_success '--set-upstream fails' ' -test_must_fail git branch --set-upstream origin/master +test_expect_success '--set-upstream-to fails' ' +test_must_fail git branch --set-upstream-to origin/master ' test_expect_success '--set-upstream-to notices an error to set branch as own upstream' ' -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails
On 5 June 2018 at 11:49, Merland Romain wrote: > >> @@ -91,6 +93,13 @@ def p4_build_cmd(cmd): >> real_cmd = ' '.join(real_cmd) + ' ' + cmd >> else: >> real_cmd += cmd >> + >> +# now check that we can actually talk to the server >> +global p4_access_checked >> +if not p4_access_checked: >> +p4_access_checked = True >> +p4_check_access() >> + > > Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()' > It seems to me more logical Like this: +p4_check_access() +p4_access_checked = True You need to set p4_access_checked first so that it doesn't go and try to check the p4 access before running "p4 login -s", which would then get stuck forever. > > Romain
Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server
On 5 June 2018 at 10:54, Eric Sunshine wrote:
> On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand wrote:
>> This change lays some groundwork for better handling of rowcount errors
>> from the server, where it fails to send us results because we requested
>> too many.
>>
>> It adds an option to p4CmdList() to return errors as a Python exception.
>>
>> The exceptions are derived from P4Exception (something went wrong),
>> P4ServerException (the server sent us an error code) and
>> P4RequestSizeException (we requested too many rows/results from the
>> server database).
>>
>> This makes makes the code that handles the errors a bit easier.
>>
>> The default behavior is unchanged; the new code is enabled with a flag.
>>
>> Signed-off-by: Luke Diamand
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -566,10 +566,30 @@ def isModeExec(mode):
>> +class P4ServerException(Exception):
>> +""" Base class for exceptions where we get some kind of marshalled up
>> result from the server """
>> +def __init__(self, exit_code, p4_result):
>> +super(P4ServerException, self).__init__(exit_code)
>> +self.p4_result = p4_result
>> +self.code = p4_result[0]['code']
>> +self.data = p4_result[0]['data']
>
> The subsequent patches never seem to access any of these fields, so
> it's difficult to judge whether it's worthwhile having 'code' and
> 'data' bits split out like this.
These changes don't use it, but I thought that future changes might
need them, and perhaps when I put that code in I was thinking I might
need it myself.
>
>> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',
>> cb=None, skip_info=False):
>> if exitCode != 0:
>> -entry = {}
>> -entry["p4ExitCode"] = exitCode
>> -result.append(entry)
>> +if errors_as_exceptions:
>> +if len(result) > 0:
>> +data = result[0].get('data')
>> +if data:
>> +m = re.search('Too many rows scanned \(over (\d+)\)',
>> data)
>> +if not m:
>> +m = re.search('Request too large \(over (\d+)\)',
>> data)
>
> Does 'p4' localize these error messages?
That's a good question.
The marshalled-up error from Perforce looks like this:
([{'generic': 35, 'code': 'error', 'data': "Too many rows scanned
(over 40); see 'p4 help maxscanrows'.\n", 'severity': 3}])
It turns out that Perforce open-sourced the P4 client in 2014 (I only
recently found this out) so we can actually look at the code now!
https://swarm.workshop.perforce.com/projects/perforce_software-p4
Clone it like this:
mkdir p4 &&
(cd p4 && git init && git config --add git-p4.branchList p4/2018-1:2018-1) &&
P4USER=guest P4PORT=workshop.perforce.com:1666 git p4 clone
--detect-branches --destination p4 //guest/perforce_software/p4@all
Here's the code:
// ErrorId graveyard: retired/deprecated ErrorIds.
ErrorId MsgDb::MaxResults = { ErrorOf( ES_DB, 32,
E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
'p4 help maxresults'." } ;//NOTRANS
ErrorId MsgDb::MaxScanRows = { ErrorOf( ES_DB, 61,
E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
see 'p4 help maxscanrows'." } ;//NOTRANS
I don't think there's actually a way to make it return any language
other than English though. There's a P4LANGUAGE environment variable,
but it just says "this is for system integrators":
https://www.perforce.com/perforce/r15.2/manuals/cmdref/P4LANGUAGE.html
So I think probably the language is always English.
Luke
Re: [PATCHv1 3/3] git-p4: auto-size the block
On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand wrote:
> git-p4 originally would fetch changes in one query. On large repos this
> could fail because of the limits that Perforce imposes on the number of
> items returned and the number of queries in the database.
>
> To fix this, git-p4 learned to query changes in blocks of 512 changes,
> However, this can be very slow - if you have a few million changes,
> with each chunk taking about a second, it can be an hour or so.
>
> Although it's possible to tune this value manually with the
> "--changes-block-size" option, it's far from obvious to ordinary users
> that this is what needs doing.
>
> This change alters the block size dynamically by looking for the
> specific error messages returned from the Perforce server, and reducing
> the block size if the error is seen, either to the limit reported by the
> server, or to half the current block size.
>
> That means we can start out with a very large block size, and then let
> it automatically drop down to a value that works without error, while
> still failing correctly if some other error occurs.
>
> Signed-off-by: Luke Diamand
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -48,7 +48,8 @@ def __str__(self):
> # Grab changes in blocks of this many revisions, unless otherwise requested
> -defaultBlockSize = 512
> +# The code should now automatically reduce the block size if it is required
> +defaultBlockSize = 1<<20
As worded, the new comment only has value to someone who knew the old
behavior (before this patch), not for someone reading the code fresh.
However, if reworded, it might be meaningful to all readers (new and
old):
# The block size is reduced automatically if required
> @@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange,
> requestedBlockSize):
> +try:
> +result = p4CmdList(cmd, errors_as_exceptions=True)
> +except P4RequestSizeException as e:
> +if not block_size:
> +block_size = e.limit
> +elif block_size > e.limit:
> +block_size = e.limit
> +else:
> +block_size = max(2, block_size // 2)
> +
> +if verbose: print("block size error, retry with block size
> {0}".format(block_size))
Perhaps: s/retry/retrying/
> diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
> @@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot
> paths' '
> +test_expect_success 'Clone repo with self-sizing block size' '
> + test_when_finished cleanup_git &&
> + git p4 clone --changes-block-size=100 //depot@all
> --destination="$git" &&
> + (
> + cd "$git" &&
> + git log --oneline > log &&
> + test_line_count \> 10 log
> + )
> +'
Or, without a subshell (and dropping whitespace after redirection operator):
git -C git log --oneline >log &&
test_line_count \> 10 log
(not worth a re-roll)
Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server
On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand wrote:
> This change lays some groundwork for better handling of rowcount errors
> from the server, where it fails to send us results because we requested
> too many.
>
> It adds an option to p4CmdList() to return errors as a Python exception.
>
> The exceptions are derived from P4Exception (something went wrong),
> P4ServerException (the server sent us an error code) and
> P4RequestSizeException (we requested too many rows/results from the
> server database).
>
> This makes makes the code that handles the errors a bit easier.
>
> The default behavior is unchanged; the new code is enabled with a flag.
>
> Signed-off-by: Luke Diamand
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -566,10 +566,30 @@ def isModeExec(mode):
> +class P4ServerException(Exception):
> +""" Base class for exceptions where we get some kind of marshalled up
> result from the server """
> +def __init__(self, exit_code, p4_result):
> +super(P4ServerException, self).__init__(exit_code)
> +self.p4_result = p4_result
> +self.code = p4_result[0]['code']
> +self.data = p4_result[0]['data']
The subsequent patches never seem to access any of these fields, so
it's difficult to judge whether it's worthwhile having 'code' and
'data' bits split out like this.
> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b',
> cb=None, skip_info=False):
> if exitCode != 0:
> -entry = {}
> -entry["p4ExitCode"] = exitCode
> -result.append(entry)
> +if errors_as_exceptions:
> +if len(result) > 0:
> +data = result[0].get('data')
> +if data:
> +m = re.search('Too many rows scanned \(over (\d+)\)',
> data)
> +if not m:
> +m = re.search('Request too large \(over (\d+)\)',
> data)
Does 'p4' localize these error messages?
> +if m:
> +limit = int(m.group(1))
> +raise P4RequestSizeException(exitCode, result, limit)
> +
> +raise P4ServerException(exitCode, result)
> +else:
> +raise P4Exception(exitCode)
> +else:
> +entry = {}
> +entry["p4ExitCode"] = exitCode
> +result.append(entry)
[PATCHv1 2/3] git-p4: narrow the scope of exceptions caught when parsing an int
The current code traps all exceptions around some code which parses an integer, and then talks to Perforce. That can result in errors from Perforce being ignored. Change the code to only catch the integer conversion exceptions. Signed-off-by: Luke Diamand --- git-p4.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index d856078ec8..b337562b39 100755 --- a/git-p4.py +++ b/git-p4.py @@ -959,7 +959,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize): try: (changeStart, changeEnd) = p4ParseNumericChangeRange(parts) block_size = chooseBlockSize(requestedBlockSize) -except: +except ValueError: changeStart = parts[0][1:] changeEnd = parts[1] if requestedBlockSize: -- 2.17.0.392.gdeb1a6e9b7
[PATCHv1 0/3] git-p4: improvements to p4 "blocking"
This patchset improves the way that git-p4 sends requests in "blocks".
The problem comes about because the Perforce server limits the maximum
number of results it will send back (there are actually two different
limits).
This causes git-p4 failures if you ask for too much data.
In commit 96b2d54aee ("git-p4: use -m when running p4 changes",
2015-04-20), git-p4 learned to make requests in blocks, by default 512
in size.
The problem with this, however, is that it can be very slow on large
repositories - you might have millions of commits, although only a
handful actually relate to the code you are trying to clone. Each 512
block request takes a sizeable fraction of a second to complete.
There's a command-line option to increase the block size, but unless you
know about it, it won't be at all obvious that you could use this to
speed things up.
This change
~~~
The function p4CmdList() has been taught to raise an exception when it
gets an error, and in particular, to notice and report the two "too many
results" errors.
The code that does the blocking can now start out with a nice large
block size, and then reduce it if it sees an error.
Luke Diamand (3):
git-p4: raise exceptions from p4CmdList based on error from p4 server
git-p4: narrow the scope of exceptions caught when parsing an int
git-p4: auto-size the block
git-p4.py | 72 +++--
t/t9818-git-p4-block.sh | 11 +++
2 files changed, 73 insertions(+), 10 deletions(-)
--
2.17.0.392.gdeb1a6e9b7
[PATCHv1 3/3] git-p4: auto-size the block
git-p4 originally would fetch changes in one query. On large repos this
could fail because of the limits that Perforce imposes on the number of
items returned and the number of queries in the database.
To fix this, git-p4 learned to query changes in blocks of 512 changes,
However, this can be very slow - if you have a few million changes,
with each chunk taking about a second, it can be an hour or so.
Although it's possible to tune this value manually with the
"--changes-block-size" option, it's far from obvious to ordinary users
that this is what needs doing.
This change alters the block size dynamically by looking for the
specific error messages returned from the Perforce server, and reducing
the block size if the error is seen, either to the limit reported by the
server, or to half the current block size.
That means we can start out with a very large block size, and then let
it automatically drop down to a value that works without error, while
still failing correctly if some other error occurs.
Signed-off-by: Luke Diamand
---
git-p4.py | 26 +-
t/t9818-git-p4-block.sh | 11 +++
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index b337562b39..6736f81b60 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -48,7 +48,8 @@ def __str__(self):
defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
# Grab changes in blocks of this many revisions, unless otherwise requested
-defaultBlockSize = 512
+# The code should now automatically reduce the block size if it is required
+defaultBlockSize = 1<<20
p4_access_checked = False
@@ -969,7 +970,8 @@ def p4ChangesForPaths(depotPaths, changeRange,
requestedBlockSize):
changes = set()
# Retrieve changes a block at a time, to prevent running
-# into a MaxResults/MaxScanRows error from the server.
+# into a MaxResults/MaxScanRows error from the server. If
+# we _do_ hit one of those errors, turn down the block size
while True:
cmd = ['changes']
@@ -983,10 +985,24 @@ def p4ChangesForPaths(depotPaths, changeRange,
requestedBlockSize):
for p in depotPaths:
cmd += ["%s...@%s" % (p, revisionRange)]
+# fetch the changes
+try:
+result = p4CmdList(cmd, errors_as_exceptions=True)
+except P4RequestSizeException as e:
+if not block_size:
+block_size = e.limit
+elif block_size > e.limit:
+block_size = e.limit
+else:
+block_size = max(2, block_size // 2)
+
+if verbose: print("block size error, retry with block size
{0}".format(block_size))
+continue
+except P4Exception as e:
+die('Error retrieving changes description
({0})'.format(e.p4ExitCode))
+
# Insert changes in chronological order
-for entry in reversed(p4CmdList(cmd)):
-if entry.has_key('p4ExitCode'):
-die('Error retrieving changes descriptions
({})'.format(entry['p4ExitCode']))
+for entry in reversed(result):
if not entry.has_key('change'):
continue
changes.add(int(entry['change']))
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 8840a183ac..e5ec9cdec8 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -129,6 +129,7 @@ test_expect_success 'Create a repo with multiple depot
paths' '
'
test_expect_success 'Clone repo with multiple depot paths' '
+ test_when_finished cleanup_git &&
(
cd "$git" &&
git p4 clone --changes-block-size=4 //depot/pathA@all
//depot/pathB@all \
@@ -138,6 +139,16 @@ test_expect_success 'Clone repo with multiple depot paths'
'
)
'
+test_expect_success 'Clone repo with self-sizing block size' '
+ test_when_finished cleanup_git &&
+ git p4 clone --changes-block-size=100 //depot@all
--destination="$git" &&
+ (
+ cd "$git" &&
+ git log --oneline > log &&
+ test_line_count \> 10 log
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.17.0.392.gdeb1a6e9b7
[PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server
This change lays some groundwork for better handling of rowcount errors
from the server, where it fails to send us results because we requested
too many.
It adds an option to p4CmdList() to return errors as a Python exception.
The exceptions are derived from P4Exception (something went wrong),
P4ServerException (the server sent us an error code) and
P4RequestSizeException (we requested too many rows/results from the
server database).
This makes makes the code that handles the errors a bit easier.
The default behavior is unchanged; the new code is enabled with a flag.
Signed-off-by: Luke Diamand
---
git-p4.py | 44
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 66652f8280..d856078ec8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,10 +566,30 @@ def isModeExec(mode):
# otherwise False.
return mode[-3:] == "755"
+class P4Exception(Exception):
+""" Base class for exceptions from the p4 client """
+def __init__(self, exit_code):
+self.p4ExitCode = exit_code
+
+class P4ServerException(Exception):
+""" Base class for exceptions where we get some kind of marshalled up
result from the server """
+def __init__(self, exit_code, p4_result):
+super(P4ServerException, self).__init__(exit_code)
+self.p4_result = p4_result
+self.code = p4_result[0]['code']
+self.data = p4_result[0]['data']
+
+class P4RequestSizeException(P4ServerException):
+""" One of the maxresults or maxscanrows errors """
+def __init__(self, exit_code, p4_result, limit):
+super(P4RequestSizeException, self).__init__(exit_code, p4_result)
+self.limit = limit
+
def isModeExecChanged(src_mode, dst_mode):
return isModeExec(src_mode) != isModeExec(dst_mode)
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
+errors_as_exceptions=False):
if isinstance(cmd,basestring):
cmd = "-G " + cmd
@@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None,
skip_info=False):
pass
exitCode = p4.wait()
if exitCode != 0:
-entry = {}
-entry["p4ExitCode"] = exitCode
-result.append(entry)
+if errors_as_exceptions:
+if len(result) > 0:
+data = result[0].get('data')
+if data:
+m = re.search('Too many rows scanned \(over (\d+)\)', data)
+if not m:
+m = re.search('Request too large \(over (\d+)\)', data)
+
+if m:
+limit = int(m.group(1))
+raise P4RequestSizeException(exitCode, result, limit)
+
+raise P4ServerException(exitCode, result)
+else:
+raise P4Exception(exitCode)
+else:
+entry = {}
+entry["p4ExitCode"] = exitCode
+result.append(entry)
return result
--
2.17.0.392.gdeb1a6e9b7
[PATCHv1 0/1] git-p4: better error reporting
If git-p4 cannot talk to the Perforce server, it will either give a confusing error message, or just crash. Even I get tripped up by this. This change just checks that the client can talk to the server, and in particular that the user is logged in (the default login timeout is 12 hours). It would be nice to also check for ticket expiration during long Perforce operations, but this change does not attempt to do that. Luke Diamand (1): git-p4: better error reporting when p4 fails git-p4.py | 55 + t/t9833-errors.sh | 78 +++ 2 files changed, 133 insertions(+) create mode 100755 t/t9833-errors.sh -- 2.17.0.392.gdeb1a6e9b7
[PATCHv1 1/1] git-p4: better error reporting when p4 fails
Currently when p4 fails to run, git-p4 just crashes with an obscure
error message.
For example, if the P4 ticket has expired, you get:
Error: Cannot locate perforce checkout of in client view
This change checks whether git-p4 can talk to the Perforce server when
the first P4 operation is attempted, and tries to print a meaningful
error message if it fails.
Signed-off-by: Luke Diamand
---
git-p4.py | 55 +
t/t9833-errors.sh | 78 +++
2 files changed, 133 insertions(+)
create mode 100755 t/t9833-errors.sh
diff --git a/git-p4.py b/git-p4.py
index b9e79f1d8b..66652f8280 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -50,6 +50,8 @@ def __str__(self):
# Grab changes in blocks of this many revisions, unless otherwise requested
defaultBlockSize = 512
+p4_access_checked = False
+
def p4_build_cmd(cmd):
"""Build a suitable p4 command line.
@@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
real_cmd = ' '.join(real_cmd) + ' ' + cmd
else:
real_cmd += cmd
+
+# now check that we can actually talk to the server
+global p4_access_checked
+if not p4_access_checked:
+p4_access_checked = True
+p4_check_access()
+
return real_cmd
def git_dir(path):
@@ -264,6 +273,52 @@ def p4_system(cmd):
if retcode:
raise CalledProcessError(retcode, real_cmd)
+def die_bad_access(s):
+die("failure accessing depot: {0}".format(s.rstrip()))
+
+def p4_check_access(min_expiration=1):
+""" Check if we can access Perforce - account still logged in
+"""
+results = p4CmdList(["login", "-s"])
+
+if len(results) == 0:
+# should never get here: always get either some results, or a
p4ExitCode
+assert("could not parse response from perforce")
+
+result = results[0]
+
+if 'p4ExitCode' in result:
+# p4 returned non-zero status, e.g. P4PORT invalid, or p4 not in path
+die_bad_access("could not run p4")
+
+code = result.get("code")
+if not code:
+# we get here if we couldn't connect and there was nothing to unmarshal
+die_bad_access("could not connect")
+
+elif code == "stat":
+expiry = result.get("TicketExpiration")
+if expiry:
+expiry = int(expiry)
+if expiry > min_expiration:
+# ok to carry on
+return
+else:
+die_bad_access("perforce ticket expires in {0}
seconds".format(expiry))
+
+else:
+# account without a timeout - all ok
+return
+
+elif code == "error":
+data = result.get("data")
+if data:
+die_bad_access("p4 error: {0}".format(data))
+else:
+die_bad_access("unknown error")
+else:
+die_bad_access("unknown error code {0}".format(code))
+
_p4_version_string = None
def p4_version_string():
"""Read the version string, showing just the last line, which
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
new file mode 100755
index 00..9ba892de7a
--- /dev/null
+++ b/t/t9833-errors.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git p4 errors'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'add p4 files' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file1"
+ )
+'
+
+# after this test, the default user requires a password
+test_expect_success 'error handling' '
+ git p4 clone --dest="$git" //depot@all &&
+ (
+ cd "$git" &&
+ P4PORT=: test_must_fail git p4 submit 2>errmsg
+ ) &&
+ p4 passwd -P newpassword &&
+ (
+ P4PASSWD=badpassword test_must_fail git p4 clone //depot/foo
2>errmsg &&
+ grep -q "failure accessing depot.*P4PASSWD" errmsg
+ )
+'
+
+test_expect_success 'ticket logged out' '
+ P4TICKETS="$cli/tickets" &&
+ echo "newpassword" | p4 login &&
+ (
+ cd "$git" &&
+ test_commit "ticket-auth-check" &&
+ p4 logout &&
+ test_must_fail git p4 submit 2>errmsg &&
+ grep -q "failure accessing depot" errmsg
+ )
+'
+
+test_expect_success 'create group with short ticket expiry' '
+ P4TICKETS="$cli/tickets" &&
+ echo "newpassword" | p4 login &&
+ p4_add_user short_expiry_user &&
+ p4 -u short_expiry_user passwd -P password &&
+ p4 group -i <<-EOF &&
+ Group: testgroup
+ Timeout: 3
+ Users: short_expiry_user
+ EOF
+
+ p4 users | grep short_expiry_user
+'
+
+test_expect_success 'git operation with expired ticket' '
+ P4TICKETS="$cli/tickets" &&
+ P4USER=short_expiry_user &&
+ echo "password" | p4 login &&
+ (
+ cd "$git" &&
+ git p4 sync &&
+
Re: [PATCHv1 2/2] git-p4: add option to disable syncing of p4/master with p4
On Tue, Jun 5, 2018 at 4:29 AM Luke Diamand wrote:
> Add an option to the git-p4 submit command to disable syncing
> with Perforce.
>
> This is useful for the case where a git-p4 mirror has been setup
> on a server somewhere, running from (e.g.) cron, and developers
> then clone from this. Having the local cloned copy also sync
> from Perforce just isn't useful.
>
> Signed-off-by: Luke Diamand
> ---
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> @@ -369,6 +369,11 @@ These options can be used to modify 'git p4 submit'
> behavior.
> +--disable-p4sync::
> +Disable the automatic sync of p4/master from Perforce after commit have
s/commit/commits/
> +been submitted. Implies --disable-rebase. Can also be set with
> +git-p4.disableP4Sync. Sync with origin/master still goes ahead if
> possible.
> diff --git a/git-p4.py b/git-p4.py
> @@ -1368,7 +1368,9 @@ def __init__(self):
> +optparse.make_option("--disable-p4sync",
> dest="disable_p4sync", action="store_true",
> + help="Skip perforce sync of p4/master
> after submit or shelve"),
s/perforce/Perforce/
[PATCHv1 2/2] git-p4: add option to disable syncing of p4/master with p4
Add an option to the git-p4 submit command to disable syncing
with Perforce.
This is useful for the case where a git-p4 mirror has been setup
on a server somewhere, running from (e.g.) cron, and developers
then clone from this. Having the local cloned copy also sync
from Perforce just isn't useful.
Signed-off-by: Luke Diamand
---
Documentation/git-p4.txt | 8
git-p4.py| 31 ---
2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 3d83842e47..8f6a7543fd 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -369,6 +369,11 @@ These options can be used to modify 'git p4 submit'
behavior.
Disable the automatic rebase after all commits have been successfully
submitted. Can also be set with git-p4.disableRebase.
+--disable-p4sync::
+Disable the automatic sync of p4/master from Perforce after commit have
+been submitted. Implies --disable-rebase. Can also be set with
+git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
+
Rebase options
~~
These options can be used to modify 'git p4 rebase' behavior.
@@ -693,6 +698,9 @@ git-p4.conflict::
git-p4.disableRebase::
Do not rebase the tree against p4/master following a submit.
+git-p4.disableP4Sync::
+Do not sync p4/master with Perforce following a submit. Implies
git-p4.disableRebase.
+
IMPLEMENTATION DETAILS
--
* Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index 5ab9421af8..b9e79f1d8b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1368,7 +1368,9 @@ def __init__(self):
help="submit only the specified
commit(s), one commit or xxx..xxx"),
optparse.make_option("--disable-rebase",
dest="disable_rebase", action="store_true",
help="Disable rebase after submit is
completed. Can be useful if you "
- "work from a local git branch that is not
master")
+ "work from a local git branch that is not
master"),
+optparse.make_option("--disable-p4sync",
dest="disable_p4sync", action="store_true",
+ help="Skip perforce sync of p4/master
after submit or shelve"),
]
self.description = "Submit changes from git to the perforce depot."
self.usage += " [name of git branch to submit into perforce depot]"
@@ -1380,6 +1382,7 @@ def __init__(self):
self.update_shelve = list()
self.commit = ""
self.disable_rebase = gitConfigBool("git-p4.disableRebase")
+self.disable_p4sync = gitConfigBool("git-p4.disableP4Sync")
self.prepare_p4_only = False
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
@@ -2240,11 +2243,14 @@ def run(self, args):
sync = P4Sync()
if self.branch:
sync.branch = self.branch
-sync.run([])
+if self.disable_p4sync:
+sync.sync_origin_only()
+else:
+sync.run([])
-if self.disable_rebase is False:
-rebase = P4Rebase()
-rebase.rebase()
+if not self.disable_rebase:
+rebase = P4Rebase()
+rebase.rebase()
else:
if len(applied) == 0:
@@ -3324,6 +3330,14 @@ def importChanges(self, changes, shelved=False,
origin_revision=0):
print self.gitError.read()
sys.exit(1)
+def sync_origin_only(self):
+if self.syncWithOrigin:
+self.hasOrigin = originP4BranchesExist()
+if self.hasOrigin:
+if not self.silent:
+print 'Syncing with origin first, using "git fetch origin"'
+system("git fetch origin")
+
def importHeadRevision(self, revision):
print "Doing initial import of %s from revision %s into %s" % ('
'.join(self.depotPaths), revision, self.branch)
@@ -3402,12 +3416,7 @@ def run(self, args):
else:
self.refPrefix = "refs/heads/p4/"
-if self.syncWithOrigin:
-self.hasOrigin = originP4BranchesExist()
-if self.hasOrigin:
-if not self.silent:
-print 'Syncing with origin first, using "git fetch origin"'
-system("git fetch origin")
+self.sync_origin_only()
branch_arg_given = bool(self.branch)
if len(self.branch) == 0:
--
2.17.0.392.gdeb1a6e9b7
[PATCHv1 0/2] git-p4: disable sync after submit
This is a small patch to git-p4 to disable the automatic sync after submit. In my day-to-day work, I have a central git-p4 repo which is automatically kept up-to-date, so the repos where I actually work and submit from don't even need the sync. I usually end up hitting Ctrl-C partway through the sync. I imagine other people with large git-p4 projects do something similar and have the same problem. This also updates Merland's recent submit-selection change so that both options can be set via configuration rather than being set on the command line. Luke Diamand (2): git-p4: disable-rebase: allow setting this via configuration git-p4: add option to disable syncing of p4/master with p4 Documentation/git-p4.txt | 13 - git-p4.py| 33 + 2 files changed, 33 insertions(+), 13 deletions(-) -- 2.17.0.392.gdeb1a6e9b7
[PATCHv1 1/2] git-p4: disable-rebase: allow setting this via configuration
This just lets you set the --disable-rebase option with the
git configuration options git-p4.disableRebase. If you're
using this option, you probably want to set it all the time
for a given repo.
Signed-off-by: Luke Diamand
---
Documentation/git-p4.txt | 5 -
git-p4.py| 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index e8452528fc..3d83842e47 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -367,7 +367,7 @@ These options can be used to modify 'git p4 submit'
behavior.
--disable-rebase::
Disable the automatic rebase after all commits have been successfully
-submitted.
+submitted. Can also be set with git-p4.disableRebase.
Rebase options
~~
@@ -690,6 +690,9 @@ git-p4.conflict::
Specify submit behavior when a conflict with p4 is found, as per
--conflict. The default behavior is 'ask'.
+git-p4.disableRebase::
+Do not rebase the tree against p4/master following a submit.
+
IMPLEMENTATION DETAILS
--
* Changesets from p4 are imported using Git fast-import.
diff --git a/git-p4.py b/git-p4.py
index c4581d20a6..5ab9421af8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1379,7 +1379,7 @@ def __init__(self):
self.shelve = False
self.update_shelve = list()
self.commit = ""
-self.disable_rebase = False
+self.disable_rebase = gitConfigBool("git-p4.disableRebase")
self.prepare_p4_only = False
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
--
2.17.0.392.gdeb1a6e9b7
Re: [RFC PATCH 4/7] merge-recursive: fix assumption that head tree being merged is HEAD
On Sun, Jun 3, 2018 at 8:19 PM, Junio C Hamano wrote: > Elijah Newren writes: > >> `git merge-recursive` does a three-way merge between user-specified trees >> base, head, and remote. Since the user is allowed to specify head, we can >> not necesarily assume that head == HEAD. >> >> We modify index_has_changes() to take an extra argument specifying the >> tree to compare the index to. If NULL, it will compare to HEAD. We then >> use this from merge-recursive to make sure we compare to the >> user-specified head. >> >> Signed-off-by: Elijah Newren >> --- >> >> I'm really unsure where the index_has_changes() declaration should go; >> I stuck it in tree.h, but is there a better spot? > > I think I saw you tried to lift an assumption that we're always > working on the_index in a separate patch recently. Should that > logic apply also to this part of the codebase? IOW, shouldn't > index_has_changes() take a pointer to istate (as opposed to a > function that uses the implicit the_index that should be named as > "cache_has_changes()" or something?) > > I tend to think this function as part of the larger read-cache.c > family whose definitions are in cache.h and accompanied by macros > that are protected by NO_THE_INDEX_COMPATIBILITY_MACROS so if we > were to move it elsewhere, I'd keep the header part as-is and > implementation to read-cache.c to keep it together with the family, > but I do not see a huge issue with the current placement, either. That's good point; the goal to lift assumptions on the_index should probably also apply here. I'll make the change. (And it was actually Duy's patch that I was reviewing, but close enough.) I'll take a look at moving it to read-cache.c as well.
