* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2012-01-08 20:07 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Malformed object name
> 3. Object name does not refer to a valid commit
>
> Since we would like to make the instruction sheet user-editable in the
> future (much like the 'rebase -i' sheet), report more fine-grained
> parse errors prefixed with the filename and line number.
Seems like a sensible idea. In other words, the infrastructure that
parses .git/sequencer/todo is meant to handle arbitrary user input
some day, so it can be used as the implementation of "git rebase
--interactive" and "git sequence --edit", and currently it is
suboptimal for that purpose because the parse error messages just say
error: Could not parse line 5.
This patch shifts responsibility to parse_insn_line(), which can come
up with a more detailed message like
error: .git/sequencer/todo:5: unrecognized action "frobnicate"
Once the operator is allowed to edit the sequence, the message might
be adjusted to something meaning
error: <sequence you just gave me>:5: unrecognized action "frobnicate"
instead of exposing an implementation detail, or maybe some day "git
sequence --edit" could even re-launch the editor with an error message
in a comment before the problematic line and the cursor pointing
there. And for now, pointing to the explicit filename is useful since
this should only happen if there was filesystem corruption, tampering,
or a git bug.
By the way, an example of the output before and after the patch would
have been useful in saving some trouble of figuring this out.
Ok, onward to the patch.
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
> return 0;
> }
>
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
> {
> + const char *todo_file = git_path(SEQ_TODO_FILE);
This idiom is _still_ scary. I don't want to have to shout about
this, but why didn't the commit message at least acknowledge it to put
the reader's mind at ease when this has come up again and again?
[...]
> + bol += strlen("revert ");
> + } else {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Unrecognized action: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
Ah, so my example above was wrong: the actual message is
error: .git/sequencer/todo:5: Unrecognized action: the quick bro
wn fox jumps over the lazy dog
I guess that's fine. Is it intended?
> + }
>
> /* Eat up extra spaces/ tabs before object name */
> - padding = strspn(bol, " \t");
> - if (!padding)
> - return -1;
> - bol += padding;
> + bol += strspn(bol, " \t");
What does this have to do with the stated goal of the patch? It seems
to me that an unrelated and unadvertised bugfix snuck in.
[...]
> @@ -741,12 +744,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> status = get_sha1(bol, commit_sha1);
> *end_of_object_name = saved;
>
> - if (status < 0)
> - return -1;
> + if (status < 0) {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Malformed object name: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
> + }
This seems a little repetitive --- maybe it would make sense to
factor out a function to emit errors of the form
error: file:lineno: message: line
By the way, this is gross. Probably get_sha1 should grow a variant
that takes a buffer and a length.
[...]
> item->operand = lookup_commit_reference(commit_sha1);
> - if (!item->operand)
> - return -1;
> + if (!item->operand) {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Not a valid commit: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
> + }
Hmm, this one can be emitted even when there was no corruption or
internal error, because the user removed a commit she was
cherry-picking and the gc-ed before a "git cherry-pick --continue".
Alternatively, it can happen because the repository has grown very
crowded and what used to be an unambiguous commit name no longer is
one (not enough digits). Will the error message be intuitive in these
situations?
[...]
> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>
> for (i = 1; *p; i++) {
> char *eol = strchrnul(p, '\n');
> - if (parse_insn_line(p, eol, &item) < 0)
> - return error(_("Could not parse line %d."), i);
> + if (parse_insn_line(p, eol, &item, i) < 0)
> + return -1;
Not related to this patch, but why the "< 0" test? It makes this
reader worry that there is some unusual return value convention that
he should be taking into account.
To sum up: the idea looks promising, but this patch doesn't seem to be
ready yet.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 20:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0kHn+SDaeRHa8bUHWvOEVkr01sVDzvnw9E+-Nne2cii1Q@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> So what change does the patch actually make? Is this a renaming?
>
> Yes, it renames "action" to "command" where appropriate.
Wouldn't a simple renaming have a diffstat with the same number of added
and removed lines?
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:03 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108193749.GH1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> When we allow mixing "revert" and "pick" instructions in the same
>> sheet in the next patch, the following workflow would be perfectly
>> valid:
>>
>> $ git cherry-pick base..latercommit
>> [conflict occurs]
>> $ edit problematicfile
>> $ git add problematicfile
>> $ git revert --continue
>> [finishes successfully]
>
> Does "workflow" mean "sequence of commands"?
Yes. Clarified wording.
>> This is confusing to the operator, because the sequencer is an
>> implementation detail hidden behind the 'git cherry-pick' and 'git
>> revert' builtins.
>
> I don't know --- it's not confusing to me. Could you explain further
> what harm the current behavior does? E.g., could it cause me to
> misunderstand some basic concepts, or could it lead me to run commands
> that cause me to scratch my head or lose data?
Junio explained this to me in [1]. It's very unnatural for a user to
want to execute "git cherry-pick --continue" when the previous command
was a "git revert": it probably means that she forgot about the
in-progress "git revert". The problem becomes more serious when the
sequencer grows more capabilities: a "git merge --continue" to
continue a "git am" sounds much more absurd. Ofcourse, we will
provide a way to continue any sequencer operation in the future: "git
continue" seems to be a good candidate.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 20:07 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200910.GK1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
>
>>> So what change does the patch actually make? Is this a renaming?
>>
>> Yes, it renames "action" to "command" where appropriate.
>
> Wouldn't a simple renaming have a diffstat with the same number of added
> and removed lines?
Yes, almost. A few extra lines added because I needed a new data enum
for the "command"; also added a convenience function: action_name().
-- Ram
^ permalink raw reply
* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Ramkumar Ramachandra @ 2012-01-08 20:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200748.GJ1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Three kinds of errors can arise from parsing '.git/sequencer/todo':
>> 1. Unrecognized action
>> 2. Malformed object name
>> 3. Object name does not refer to a valid commit
>>
>> Since we would like to make the instruction sheet user-editable in the
>> future (much like the 'rebase -i' sheet), report more fine-grained
>> parse errors prefixed with the filename and line number.
>
> Seems like a sensible idea. In other words,
> [...]
Thanks for the new wording.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>> return 0;
>> }
>>
>> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>> +static int parse_insn_line(char *bol, char *eol,
>> + struct replay_insn_list *item, int lineno)
>> {
>> + const char *todo_file = git_path(SEQ_TODO_FILE);
>
> This idiom is _still_ scary. I don't want to have to shout about
> this, but why didn't the commit message at least acknowledge it to put
> the reader's mind at ease when this has come up again and again?
Carried over from the previous re-roll; sorry I didn't pay enough attention.
> [...]
>> + bol += strlen("revert ");
>> + } else {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + return error(_("%s:%d: Unrecognized action: %.*s"),
>> + todo_file, lineno, (int)error_len, bol);
>
> Ah, so my example above was wrong: the actual message is
>
> error: .git/sequencer/todo:5: Unrecognized action: the quick bro
> wn fox jumps over the lazy dog
>
> I guess that's fine. Is it intended?
>
>> + }
>>
>> /* Eat up extra spaces/ tabs before object name */
>> - padding = strspn(bol, " \t");
>> - if (!padding)
>> - return -1;
>> - bol += padding;
>> + bol += strspn(bol, " \t");
>
> What does this have to do with the stated goal of the patch? It seems
> to me that an unrelated and unadvertised bugfix snuck in.
Not a bugfix: since I have to report sensible error messages now, I
changed the "pick" and "revert" checks to "pick " || "pick\t" and
"revert " || "revert\t" -- then, I can report "invalid action" if it
doesn't match instead of a useless "missing space after action".
> [...]
> By the way, this is gross. Probably get_sha1 should grow a variant
> that takes a buffer and a length.
Yes; will do.
> [...]
>> item->operand = lookup_commit_reference(commit_sha1);
>> - if (!item->operand)
>> - return -1;
>> + if (!item->operand) {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + return error(_("%s:%d: Not a valid commit: %.*s"),
>> + todo_file, lineno, (int)error_len, bol);
>> + }
>
> Hmm, this one can be emitted even when there was no corruption or
> internal error, because the user removed a commit she was
> cherry-picking and the gc-ed before a "git cherry-pick --continue".
> Alternatively, it can happen because the repository has grown very
> crowded and what used to be an unambiguous commit name no longer is
> one (not enough digits). Will the error message be intuitive in these
> situations?
Something like "Unable to look up commit: %s" perhaps?
> [...]
>> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>>
>> for (i = 1; *p; i++) {
>> char *eol = strchrnul(p, '\n');
>> - if (parse_insn_line(p, eol, &item) < 0)
>> - return error(_("Could not parse line %d."), i);
>> + if (parse_insn_line(p, eol, &item, i) < 0)
>> + return -1;
>
> Not related to this patch, but why the "< 0" test? It makes this
> reader worry that there is some unusual return value convention that
> he should be taking into account.
Right. Will fix.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 20:22 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=-AWy7HnVASB1rt8njavTYOhV7Zxsdq4TE+VShVZmEzQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> I don't know --- it's not confusing to me. Could you explain further
>> what harm the current behavior does? E.g., could it cause me to
>> misunderstand some basic concepts, or could it lead me to run commands
>> that cause me to scratch my head or lose data?
>
> Junio explained this to me in [1]. It's very unnatural for a user to
> want to execute "git cherry-pick --continue" when the previous command
> was a "git revert": it probably means that she forgot about the
> in-progress "git revert".
[...]
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
I don't think that's what Junio said.
Did this actually happen, or is it a theoretical worry? I think I would
be more likely to run "git cherry-pick <foo>..<bar>" than "git
cherry-pick --continue" if I had forgotten about an in-progress
revert. The former already errors out with a sensible message.
Or is the problem that I might run:
git revert foo..bar
git reset --merge; # conflict --- let's clean this up
# ah, I remember reverting the patch that conflicted before;
# let's reuse the resolution.
git cherry-pick baz
edit file.c; # another conflict, sigh
git add file.c
git cherry-pick --continue; # oops!
? That seems like a real worry, but the same problem could happen
with cherry-pick used both for the multipick and single-pick, so I
don't think your patch fundamentally addresses it.
In other words, this is a problem caused by the overloading of the
same cherry-pick command for single-pick and multi-pick. I think it
should be preventable by remembering which action failed when stopping
a sequence and doing only a single-pick resume if
CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.
The "oops" is bad since the operator might have been intending to run
some more tests and amend as necessary before continuing the
multi-pick. It is not _that_ bad, since more typically one would have
already run some tests before running cherry-pick --continue to commit
the resolution. Still probably worth fixing.
> The problem becomes more serious when the
> sequencer grows more capabilities: a "git merge --continue" to
> continue a "git am" sounds much more absurd. Ofcourse, we will
> provide a way to continue any sequencer operation in the future: "git
> continue" seems to be a good candidate.
I don't understand why "cherry-pick --continue" resuming a revert
sequence implies that "merge --continue" would have to as well.
All that said, forbidding cherry-pick --continue from resuming a
revert sequence would be fine with me, _as long as the semantics are
clearly spelled out in the commit message and documentation_. What
happens when there is a mixture of picks and reverts?
Thanks.
Jonathan
^ permalink raw reply
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Ramkumar Ramachandra @ 2012-01-08 20:17 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108194014.GI1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Parse the instruction sheet in '.git/sequencer/todo' as a list of
>> (action, operand) pairs, instead of assuming that all lines have the
>> same action.
>
> Yes, I've always liked this one.
>
> Haven't re-read the patch to avoid wasted effort if there are changes
> when the previous patches in the series change. Maybe it would be
> possible to send as a standalone?
If I don't get manage to get the series right in a couple of re-rolls,
I'll do that.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:28 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108202216.GL1942@burratino>
Hi again,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
> [...]
>> Junio explained this to me in [1]. It's very unnatural for a user to
>> want to execute "git cherry-pick --continue" when the previous command
>> was a "git revert": it probably means that she forgot about the
>> in-progress "git revert".
> [...]
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
>
> I don't think that's what Junio said.
>
> Did this actually happen, or is it a theoretical worry? I think I would
> be more likely to run "git cherry-pick <foo>..<bar>" than "git
> cherry-pick --continue" if I had forgotten about an in-progress
> revert. The former already errors out with a sensible message.
>
> Or is the problem that I might run:
>
> git revert foo..bar
> git reset --merge; # conflict --- let's clean this up
>
> # ah, I remember reverting the patch that conflicted before;
> # let's reuse the resolution.
> git cherry-pick baz
> edit file.c; # another conflict, sigh
> git add file.c
> git cherry-pick --continue; # oops!
>
> ? That seems like a real worry, but the same problem could happen
> with cherry-pick used both for the multipick and single-pick, so I
> don't think your patch fundamentally addresses it.
Good catch. I didn't replay this scenario in my head earlier.
> In other words, this is a problem caused by the overloading of the
> same cherry-pick command for single-pick and multi-pick. I think it
> should be preventable by remembering which action failed when stopping
> a sequence and doing only a single-pick resume if
> CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.
I was attempting to fix this to simplify the life of the user, not
complicate it further- the user might have no idea what the next
command in the sequence is, and I don't see the point in
inconveniencing her. In retrospect, I think we should simply drop
this patch.
> What
> happens when there is a mixture of picks and reverts?
Permitted.
-- Ram
^ permalink raw reply
* Re: [PATCH 6/6] sequencer: factor code out of revert builtin
From: Jonathan Nieder @ 2012-01-08 20:38 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-7-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Expose the cherry-picking machinery through a public
> sequencer_pick_revisions() (renamed from pick_revisions() in
> builtin/revert.c), so that cherry-picking and reverting are special
> cases of a general sequencer operation. The cherry-pick builtin is
> now a thin wrapper that does command-line argument parsing before
> calling into sequencer_pick_revisions(). In the future, we can write
> a new "foo" builtin that calls into the sequencer like:
Nice.
I don't think the plan is actually a "foo" builtin.
I guess I would say "So now your 'foo' builtin can use the sequencer
machinery by implementing a 'parse_args_populate_opts' function and
then running the following:" so as to leave the reader more excited
and not to imply a promise we are not going to keep. :)
> memset(&opts, 0, sizeof(opts));
> opts.command = REPLAY_CMD_FOO;
> opts.revisions = xmalloc(sizeof(*opts.revs));
> parse_args_populate_opts(argc, argv, &opts);
> init_revisions(opts.revs);
> sequencer_pick_revisions(&opts);
Hm, I wonder if opts.command should be a string so each new caller
does not have to add to the enum and switch statements...
> This patch does not intend to make any functional changes. Check
> with:
>
> $ git blame -s -C HEAD^..HEAD -- sequencer.c
Neat.
[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,29 @@ enum replay_subcommand {
> REPLAY_ROLLBACK
> };
>
> +struct replay_opts {
[...]
> +};
[...]
> @@ -33,4 +56,6 @@ struct replay_insn_list {
[...]
> +int sequencer_pick_revisions(struct replay_opts *opts);
Looks sensible. Maybe it would be useful to include a
Documentation/technical/api-sequencer.txt explaining how this is meant
to be used. (Not much detail needed, just an overview. See the
surrounding files for some examples.)
Haven't checked the code movement yet, since it is sitting on top of a
slushy base. Next time, hopefully.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/6] The move to sequencer.c
From: Jonathan Nieder @ 2012-01-08 20:43 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0=xhnXq4_uEAri74Kk9zbAgiS+z-nG7Etm3MCo2cXaNPw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> For the sake of new and forgetful readers: what is that objective?
>
> The objective is to build a generalized sequencer
[...]
> In these examples, the instruction sheet is uniformly filled with
> "pick" or "revert" actions, which is not very interesting. When we
> get an interface to allow easy editing of the instruction sheet and
> encompass more builtins, more interesting sequences of operations will
> be possible like:
>
> pick rr/revert-cherry-pick^2~34
> revert master@{12}
> merge next
> am /tmp/jrn.patch
Ah, okay. So the tasks at hand are (1) allowing heterogeneous
instruction sheets (pick + revert for now) and (2) exposing the
sequencer interface in sequencer.h. (2) does not strictly
depend on (1) but (1) has more short-term benefit that is easy
to test so it comes first. No UI aside from editing the instruction
sheet manually for now, but that can come later. Thanks for
explaining.
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 20:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0kwbzXRyFf=JjfAW9yD7M_FTB80+q1UPOCv-Em4qO2RKQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> In retrospect, I think we should simply drop
> this patch.
Great --- I don't think it had much to do with this series.
I'll keep this mail in my inbox. Who knows --- maybe I'll get a
moment to teach conflicted single-picks to write a
.git/sequencer/when-continuing-just-continue-the-single-pick file.
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 20:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=kw=EEXDfnrFkeNV678r4u3v72-=hKh9w8ujRN125NPQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> Jonathan Nieder wrote:
>>>> So what change does the patch actually make? Is this a renaming?
>>>
>>> Yes, it renames "action" to "command" where appropriate.
>>
>> Wouldn't a simple renaming have a diffstat with the same number of added
>> and removed lines?
>
> Yes, almost. A few extra lines added because I needed a new data enum
> for the "command"; also added a convenience function: action_name().
It's not a simple renaming, then.
What user-visible effect will this have, if any? What programmer-visible
effect will it have, if any? I _really_ should not have to read the patch
to learn the impact of a patch.
^ permalink raw reply
* [PATCH 2/5 v2] dashed externals: kill children on exit
From: Clemens Buchacher @ 2012-01-08 20:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7v4nw6hfpy.fsf@alter.siamese.dyndns.org>
Several git commands are so-called dashed externals, that is commands
executed as a child process of the git wrapper command. If the git
wrapper is killed by a signal, the child process will continue to run.
This is different from internal commands, which always die with the git
wrapper command.
Enable the recently introduced cleanup mechanism for child processes in
order to make dashed externals act more in line with internal commands.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sat, Jan 07, 2012 at 10:26:49PM -0800, Junio C Hamano wrote:
>
> Yeah, I agree 100% with that reasoning. I seem to recall that was how this
> commit was done in what I privately reviewed after Clemens announced his
> github branch?
What I had previously enabled child cleanup for all callers of
run_command_v_opt. There is quite a few of those as well, most of them
not related to dashed externals at all.
So I reworked that patch a bit to enable cleanup only for dashed
externals. This is a replacement for "[PATCH 2/5] run-command: kill
children on exit by default".
I also re-added Jeff's "send-pack: kill pack-objects helper on signal or
exit" and I dropped the extraneous newline that Erik spotted in
"[PATCH 1/5] run-command: optionally kill children on exit".
I have pushed the reworked series to cb/git-daemon-tests on my github
repo.
git.c | 2 +-
run-command.c | 1 +
run-command.h | 1 +
3 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/git.c b/git.c
index fb9029c..3805616 100644
--- a/git.c
+++ b/git.c
@@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv)
* if we fail because the command is not found, it is
* OK to return. Otherwise, we just pass along the status code.
*/
- status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
if (status >= 0 || errno != ENOENT)
exit(status);
diff --git a/run-command.c b/run-command.c
index fff9073..90bfd8c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -496,6 +496,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+ cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
}
int run_command_v_opt(const char **argv, int opt)
diff --git a/run-command.h b/run-command.h
index 2a69466..44f7d2b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...);
#define RUN_COMMAND_STDOUT_TO_STDERR 4
#define RUN_SILENT_EXEC_FAILURE 8
#define RUN_USING_SHELL 16
+#define RUN_CLEAN_ON_EXIT 32
int run_command_v_opt(const char **argv, int opt);
/*
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/5] run-command: optionally kill children on exit
From: Clemens Buchacher @ 2012-01-08 20:56 UTC (permalink / raw)
To: Erik Faye-Lund
Cc: Junio C Hamano, git, Jeff King, Jonathan Nieder, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CABPQNSb57LA6dYJvT7xF_vFfBFqKhCMbrQYp49_Ko1WmbUnYPw@mail.gmail.com>
On Sat, Jan 07, 2012 at 01:45:03PM +0100, Erik Faye-Lund wrote:
>
> Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
> supports SIGKILL, so propagating other signals to child-processes will
> fail with EINVAL. That being said, Windows' support for signals is
> severely limited, but I'm not entirely sure which ones can be
> generated in this case.
On Linux at least, SIGKILL is not a viable alternative for SIGTERM,
since it does not give the child process to do any cleanup of its own
(such as signaling its own children, for example).
In any case, due this whole experience, and recently another one with
overzealous virus scanners, I have added a "get rid of dashed externals"
work item to my TODO list.
^ permalink raw reply
* Re: [PATCH 2/5 v2] dashed externals: kill children on exit
From: Jeff King @ 2012-01-08 21:07 UTC (permalink / raw)
To: Clemens Buchacher
Cc: Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120108204109.GA3394@ecki.lan>
On Sun, Jan 08, 2012 at 09:41:09PM +0100, Clemens Buchacher wrote:
> What I had previously enabled child cleanup for all callers of
> run_command_v_opt. There is quite a few of those as well, most of them
> not related to dashed externals at all.
>
> So I reworked that patch a bit to enable cleanup only for dashed
> externals. This is a replacement for "[PATCH 2/5] run-command: kill
> children on exit by default".
Thanks, this looks much closer to what I was expecting.
-Peff
^ permalink raw reply
* enable push --quiet
From: Clemens Buchacher @ 2012-01-08 21:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi Junio,
This is a resend of the push --quiet bugfix "series".
[PATCH 1/3] server_supports(): parse feature list more carefully
[PATCH 2/3] fix push --quiet: add 'quiet' capability to receive-pack
[PATCH 3/3] t5541: avoid TAP test miscounting
I think it was looking good except for the TAP issue that you reported in this
email [1], which Michael "fixes" in PATCH 3/3, but which I could not reproduce
and which was never really understood.
The series is also available as branch cb/push-quiet at
http://github.com/drizzd/git .
Clemens
[1] http://mid.gmane.org/7v62l7crpg.fsf@alter.siamese.dyndns.org
^ permalink raw reply
* [PATCH 1/3] server_supports(): parse feature list more carefully
From: Clemens Buchacher @ 2012-01-08 21:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1326056781-17456-1-git-send-email-drizzd@aon.at>
From: Junio C Hamano <gitster@pobox.com>
We have been carefully choosing feature names used in the protocol
extensions so that the vocabulary does not contain a word that is a
substring of another word, so it is not a real problem, but we have
recently added "quiet" feature word, which would mean we cannot later
add some other word with "quiet" (e.g. "quiet-push"), which is awkward.
Let's make sure that we can eventually be able to do so by teaching the
clients and servers that feature words consist of non whitespace
letters. This parser also allows us to later add features with parameters
e.g. "feature=1.5" (parameter values need to be quoted for whitespaces,
but we will worry about the detauls when we do introduce them).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/receive-pack.c | 5 +++--
cache.h | 1 +
connect.c | 23 +++++++++++++++++++++--
upload-pack.c | 22 +++++++++++++---------
4 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d2dcb7e..d8ddcaa 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -731,9 +731,10 @@ static struct command *read_head_info(void)
refname = line + 82;
reflen = strlen(refname);
if (reflen + 82 < len) {
- if (strstr(refname + reflen + 1, "report-status"))
+ const char *feature_list = refname + reflen + 1;
+ if (parse_feature_request(feature_list, "report-status"))
report_status = 1;
- if (strstr(refname + reflen + 1, "side-band-64k"))
+ if (parse_feature_request(feature_list, "side-band-64k"))
use_sideband = LARGE_PACKET_MAX;
}
cmd = xcalloc(1, sizeof(struct command) + len - 80);
diff --git a/cache.h b/cache.h
index 10afd71..9bd8c2d 100644
--- a/cache.h
+++ b/cache.h
@@ -1037,6 +1037,7 @@ struct extra_have_objects {
};
extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
extern int server_supports(const char *feature);
+extern const char *parse_feature_request(const char *features, const char *feature);
extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
diff --git a/connect.c b/connect.c
index 2ea5c3c..912cdde 100644
--- a/connect.c
+++ b/connect.c
@@ -101,8 +101,27 @@ struct ref **get_remote_heads(int in, struct ref **list,
int server_supports(const char *feature)
{
- return server_capabilities &&
- strstr(server_capabilities, feature) != NULL;
+ return !!parse_feature_request(server_capabilities, feature);
+}
+
+const char *parse_feature_request(const char *feature_list, const char *feature)
+{
+ int len;
+
+ if (!feature_list)
+ return NULL;
+
+ len = strlen(feature);
+ while (*feature_list) {
+ const char *found = strstr(feature_list, feature);
+ if (!found)
+ return NULL;
+ if ((feature_list == found || isspace(found[-1])) &&
+ (!found[len] || isspace(found[len]) || found[len] == '='))
+ return found;
+ feature_list = found + 1;
+ }
+ return NULL;
}
enum protocol {
diff --git a/upload-pack.c b/upload-pack.c
index 6f36f62..0d11e21 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -585,6 +585,7 @@ static void receive_needs(void)
write_str_in_full(debug_fd, "#S\n");
for (;;) {
struct object *o;
+ const char *features;
unsigned char sha1_buf[20];
len = packet_read_line(0, line, sizeof(line));
reset_timeout();
@@ -616,23 +617,26 @@ static void receive_needs(void)
get_sha1_hex(line+5, sha1_buf))
die("git upload-pack: protocol error, "
"expected to get sha, not '%s'", line);
- if (strstr(line+45, "multi_ack_detailed"))
+
+ features = line + 45;
+
+ if (parse_feature_request(features, "multi_ack_detailed"))
multi_ack = 2;
- else if (strstr(line+45, "multi_ack"))
+ else if (parse_feature_request(features, "multi_ack"))
multi_ack = 1;
- if (strstr(line+45, "no-done"))
+ if (parse_feature_request(features, "no-done"))
no_done = 1;
- if (strstr(line+45, "thin-pack"))
+ if (parse_feature_request(features, "thin-pack"))
use_thin_pack = 1;
- if (strstr(line+45, "ofs-delta"))
+ if (parse_feature_request(features, "ofs-delta"))
use_ofs_delta = 1;
- if (strstr(line+45, "side-band-64k"))
+ if (parse_feature_request(features, "side-band-64k"))
use_sideband = LARGE_PACKET_MAX;
- else if (strstr(line+45, "side-band"))
+ else if (parse_feature_request(features, "side-band"))
use_sideband = DEFAULT_PACKET_MAX;
- if (strstr(line+45, "no-progress"))
+ if (parse_feature_request(features, "no-progress"))
no_progress = 1;
- if (strstr(line+45, "include-tag"))
+ if (parse_feature_request(features, "include-tag"))
use_include_tag = 1;
o = lookup_object(sha1_buf);
--
1.7.8
^ permalink raw reply related
* [PATCH 3/3] t5541: avoid TAP test miscounting
From: Clemens Buchacher @ 2012-01-08 21:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael J Gruber
In-Reply-To: <1326056781-17456-1-git-send-email-drizzd@aon.at>
From: Michael J Gruber <git@drmicha.warpmail.net>
lib-terminal.sh runs a test and thus increases the test count, but the
output is lost so that TAP produces a "no plan found error".
Move the lib-terminal call after the lib-httpd and make TAP happy
(though still leave me clueless).
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/t5541-http-push.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 0c3cd3b..6c9ec6f 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -5,7 +5,6 @@
test_description='test smart pushing over http via http-backend'
. ./test-lib.sh
-. "$TEST_DIRECTORY"/lib-terminal.sh
if test -n "$NO_CURL"; then
skip_all='skipping test, git built without http support'
@@ -15,6 +14,7 @@ fi
ROOT_PATH="$PWD"
LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5541'}
. "$TEST_DIRECTORY"/lib-httpd.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
start_httpd
test_expect_success 'setup remote repository' '
--
1.7.8
^ permalink raw reply related
* [PATCH 2/3] fix push --quiet: add 'quiet' capability to receive-pack
From: Clemens Buchacher @ 2012-01-08 21:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Todd Zullinger
In-Reply-To: <1326056781-17456-1-git-send-email-drizzd@aon.at>
Currently, git push --quiet produces some non-error output, e.g.:
$ git push --quiet
Unpacking objects: 100% (3/3), done.
This fixes a bug reported for the fedora git package:
https://bugzilla.redhat.com/show_bug.cgi?id=725593
Reported-by: Jesse Keating <jkeating@redhat.com>
Cc: Todd Zullinger <tmz@pobox.com>
Commit 90a6c7d4 (propagate --quiet to send-pack/receive-pack)
introduced the --quiet option to receive-pack and made send-pack
pass that option. Older versions of receive-pack do not recognize
the option, however, and terminate immediately. The commit was
therefore reverted.
This change instead adds a 'quiet' capability to receive-pack,
which is a backwards compatible.
In addition, this fixes push --quiet via http: A verbosity of 0
means quiet for remote helpers.
Reported-by: Tobias Ulmer <tobiasu@tmux.org>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/receive-pack.c | 14 ++++++++++++--
builtin/send-pack.c | 13 ++++++++++---
remote-curl.c | 4 +++-
t/t5523-push-upstream.sh | 7 +++++++
t/t5541-http-push.sh | 8 ++++++++
5 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d8ddcaa..31d17cf 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -33,6 +33,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
@@ -122,7 +123,7 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void
else
packet_write(1, "%s %s%c%s%s\n",
sha1_to_hex(sha1), path, 0,
- " report-status delete-refs side-band-64k",
+ " report-status delete-refs side-band-64k quiet",
prefer_ofs_delta ? " ofs-delta" : "");
sent_capabilities = 1;
return 0;
@@ -736,6 +737,8 @@ static struct command *read_head_info(void)
report_status = 1;
if (parse_feature_request(feature_list, "side-band-64k"))
use_sideband = LARGE_PACKET_MAX;
+ if (parse_feature_request(feature_list, "quiet"))
+ quiet = 1;
}
cmd = xcalloc(1, sizeof(struct command) + len - 80);
hashcpy(cmd->old_sha1, old_sha1);
@@ -789,8 +792,10 @@ static const char *unpack(void)
if (ntohl(hdr.hdr_entries) < unpack_limit) {
int code, i = 0;
- const char *unpacker[4];
+ const char *unpacker[5];
unpacker[i++] = "unpack-objects";
+ if (quiet)
+ unpacker[i++] = "-q";
if (fsck_objects)
unpacker[i++] = "--strict";
unpacker[i++] = hdr_arg;
@@ -904,6 +909,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
const char *arg = *argv++;
if (*arg == '-') {
+ if (!strcmp(arg, "--quiet")) {
+ quiet = 1;
+ continue;
+ }
+
if (!strcmp(arg, "--advertise-refs")) {
advertise_refs = 1;
continue;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index cd1115f..71f258e 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -263,6 +263,8 @@ int send_pack(struct send_pack_args *args,
args->use_ofs_delta = 1;
if (server_supports("side-band-64k"))
use_sideband = 1;
+ if (!server_supports("quiet"))
+ args->quiet = 0;
if (!remote_refs) {
fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -301,11 +303,12 @@ int send_pack(struct send_pack_args *args,
char *old_hex = sha1_to_hex(ref->old_sha1);
char *new_hex = sha1_to_hex(ref->new_sha1);
- if (!cmds_sent && (status_report || use_sideband)) {
- packet_buf_write(&req_buf, "%s %s %s%c%s%s",
+ if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
+ packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
old_hex, new_hex, ref->name, 0,
status_report ? " report-status" : "",
- use_sideband ? " side-band-64k" : "");
+ use_sideband ? " side-band-64k" : "",
+ args->quiet ? " quiet" : "");
}
else
packet_buf_write(&req_buf, "%s %s %s",
@@ -439,6 +442,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
args.force_update = 1;
continue;
}
+ if (!strcmp(arg, "--quiet")) {
+ args.quiet = 1;
+ continue;
+ }
if (!strcmp(arg, "--verbose")) {
args.verbose = 1;
continue;
diff --git a/remote-curl.c b/remote-curl.c
index 48c20b8..bcbc7fb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -770,7 +770,9 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
argv[argc++] = "--thin";
if (options.dry_run)
argv[argc++] = "--dry-run";
- if (options.verbosity > 1)
+ if (options.verbosity == 0)
+ argv[argc++] = "--quiet";
+ else if (options.verbosity > 1)
argv[argc++] = "--verbose";
argv[argc++] = url;
for (i = 0; i < nr_spec; i++)
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index c229fe6..9ee52cf 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -108,4 +108,11 @@ test_expect_failure TTY 'push --no-progress suppresses progress' '
! grep "Writing objects" err
'
+test_expect_success TTY 'quiet push' '
+ ensure_fresh_upstream &&
+
+ test_terminal git push --quiet --no-progress upstream master 2>&1 | tee output &&
+ test_cmp /dev/null output
+'
+
test_done
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 9b85d42..0c3cd3b 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -5,6 +5,7 @@
test_description='test smart pushing over http via http-backend'
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
if test -n "$NO_CURL"; then
skip_all='skipping test, git built without http support'
@@ -186,5 +187,12 @@ test_expect_success 'push --mirror to repo with alternates' '
git push --mirror "$HTTPD_URL"/smart/alternates-mirror.git
'
+test_expect_success TTY 'quiet push' '
+ cd "$ROOT_PATH"/test_repo_clone &&
+ test_commit quiet &&
+ test_terminal git push --quiet --no-progress 2>&1 | tee output &&
+ test_cmp /dev/null output
+'
+
stop_httpd
test_done
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2012-01-08 21:33 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0mStgcb4EBB+ni9fisDJY=13cJZWCTEcgfyOUyAXbc=tA@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> /* Eat up extra spaces/ tabs before object name */
>>> - padding = strspn(bol, " \t");
>>> - if (!padding)
>>> - return -1;
>>> - bol += padding;
>>> + bol += strspn(bol, " \t");
[...]
> Not a bugfix: since I have to report sensible error messages now, I
> changed the "pick" and "revert" checks to "pick " || "pick\t" and
> "revert " || "revert\t" -- then, I can report "invalid action" if it
> doesn't match instead of a useless "missing space after action".
Ah, I forgot that the "if (!padding)" check noticed errors like
picking foo
before. So this is just a code cleanup, with no functional effect.
However, you can still report "invalid action" with the old code
structure --- it would just mean duplicating an error message in the
code, since you reach the same conclusion by two code paths. So it's
a relevant cleanup, but I'd still suggest lifting it into a patch that
comes before so future readers can assure themselves that it introduces
no functional change instead of being confused.
[...]
>>> + return error(_("%s:%d: Not a valid commit: %.*s"),
>>> + todo_file, lineno, (int)error_len, bol);
>>> + }
>>
>> Hmm, this one can be emitted even when there was no corruption or
>> internal error, because the user removed a commit she was
>> cherry-picking and the gc-ed before a "git cherry-pick --continue".
>> Alternatively, it can happen because the repository has grown very
>> crowded and what used to be an unambiguous commit name no longer is
>> one (not enough digits). Will the error message be intuitive in these
>> situations?
>
> Something like "Unable to look up commit: %s" perhaps?
My "alternatively" was bogus --- lookup_commit_reference takes a (raw)
full commit name as its argument.
I dunno. The question was not actually rhetorical --- I just meant
that it's worth thinking about these cases. There are a few cases:
- missing object
- object is present but corrupt
- object is a blob, not a commit
In the second case, there's an error message printed describing the
problem, but in the other two there isn't. The other callers tend to
say "not a valid <foo>" or "could not lookup commit <foo>, so I guess
error: .git/sequencer/todo:5: not a valid commit: 78a89f493
would be fine.
Except that this focusses on the .git/sequencer/todo filename which
would leave the person debugging astray. It is not that
.git/sequencer/todo contains a typo (that would have been caught by
get_sha1), but that it referred to a bad object or non-commit. Maybe
something in the direction of
error: cannot pick 78a89f493 because it is not a valid commit
would be more helpful.
Is this the right moment to report that error? Will the operator be
happy that we errored out right away before cherry-picking anything
and wasting the human's time in assisting with that process, or will
she be unhappy that inability to do something later that she might
have been planning on skipping anyway prevented making progress right
away? (I'm not sure what the best thing to do is --- I guess some
advice like
hint: to abort, use cherry-pick --abort
hint: to skip or replace that commit, use cherry-pick --edit
would help.)
Thanks for some food for thought.
Jonathan
^ permalink raw reply
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Jonathan Nieder @ 2012-01-08 21:40 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0mYbBsZN1UX9YM0VWQezZsBpJCcEgKirCggtNXs0HZ-8g@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Yes, I've always liked this one.
>>
>> Haven't re-read the patch to avoid wasted effort if there are changes
>> when the previous patches in the series change. Maybe it would be
>> possible to send as a standalone?
>
> If I don't get manage to get the series right in a couple of re-rolls,
> I'll do that.
Splitting out unrelated changes (or at least putting the uncontroversial
ones near the front) is part of getting a series right. I'll pick up
this patch, play around with it and send separately.
^ permalink raw reply
* [PATCH] rebase --fix: interactive fixup mode
From: Clemens Buchacher @ 2012-01-08 21:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Interactive rebase is frequently used not to rebase history, but to
manipulate recent commits. This is typically done using the following
command:
git rebase -i HEAD~N
Where N has to be large enough such that the the range HEAD~N..HEAD
contains the desired commits. At the same time, it should be small
enough such that the range HEAD~N..HEAD does not include published
commits or a merge commit. Otherwise, the user may accidentally change
published history. Rebasing a merge commit can also have the generally
undesirable effect of linearizing the merge history.
In order to determine a suitable range automatically, it is a reasonable
heuristic to rebase onto the most recent merge commit. It does not
guarantee that published commits are not included -- indeed there is no
way to do that. But, the range is usually large enough to contain the
desired commits. Also, this mechanism works regardless of whether or not
branch tracking has been configured.
So instead of the above command, one can instead use the following:
git rebase --fix
By default, the range is limited to a maximum of 20 commits. This can be
changed by passing a different number to --fix, e.g.:
git rebase --fix=50
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Also on branch cb/rebase-fix at https://github.com/drizzd/git .
Documentation/git-rebase.txt | 9 +++++++++
git-rebase.sh | 37 ++++++++++++++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 504945c..b1eac16 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -332,6 +332,15 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).
user edit that list before rebasing. This mode can also be used to
split commits (see SPLITTING COMMITS below).
+--fix=<n>::
+ Searches commit history backwards from the current commit until the
+ most recent merge commit, or until a maximum of <n> preceding commits
+ (default: 20), and runs rebase -i <commit>^. The resulting range is
+ typically large enough to contain recent commits which the user might
+ want to edit, while avoiding the usually undesirable effects of
+ rebasing a merge commit, which obviates the need to find a suitable
+ base commit manually.
+
-p::
--preserve-merges::
Instead of ignoring merges, try to recreate them.
diff --git a/git-rebase.sh b/git-rebase.sh
index 00ca7b9..e95b57f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -38,6 +38,7 @@ git-rebase [-i] --continue | --abort | --skip
v,verbose! display a diffstat of what changed upstream
q,quiet! be quiet. implies --no-stat
onto=! rebase onto given branch instead of upstream
+fix?! interactive rebase onto last merge commit
p,preserve-merges! try to recreate merges instead of ignoring them
s,strategy=! use the given merge strategy
no-ff! cherry-pick all commits, even if unchanged
@@ -95,6 +96,7 @@ type=
state_dir=
# One of {'', continue, skip, abort}, as parsed from command line
action=
+rebase_fix=
preserve_merges=
autosquash=
test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
@@ -178,6 +180,22 @@ run_pre_rebase_hook () {
fi
}
+latest_merge_commit()
+{
+ max_nr_commits=$1
+
+ latest_merge=$(git rev-list -1 --merges HEAD)
+ if test -z "$latest_merge"
+ then
+ range=HEAD
+ else
+ range=$latest_merge..HEAD
+ fi
+
+ range_start=$(git rev-list -"$max_nr_commits" "$range" | tail -1)
+ echo $(git rev-parse $range_start^)
+}
+
test -f "$apply_dir"/applying &&
die 'It looks like git-am is in progress. Cannot rebase.'
@@ -220,6 +238,20 @@ do
-i)
interactive_rebase=explicit
;;
+ --fix)
+ interactive_rebase=explicit
+ rebase_fix=20
+ # Parse optional argument.
+ if test "${2#-}" = "$2"
+ then
+ if ! expr "$2" : "^[0-9]\+$" >/dev/null
+ then
+ die "Invalid argument to rebase --fix: $2"
+ fi
+ rebase_fix=$2
+ shift
+ fi
+ ;;
-p)
preserve_merges=t
test -z "$interactive_rebase" && interactive_rebase=implied
@@ -375,7 +407,10 @@ if test -z "$rebase_root"
then
case "$#" in
0)
- if ! upstream_name=$(git rev-parse --symbolic-full-name \
+ if test -n "$rebase_fix"
+ then
+ upstream_name=$(latest_merge_commit $rebase_fix)
+ elif ! upstream_name=$(git rev-parse --symbolic-full-name \
--verify -q @{upstream} 2>/dev/null)
then
. git-parse-remote
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Jonathan Nieder @ 2012-01-08 21:55 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108214042.GR1942@burratino>
Jonathan Nieder wrote:
> I'll pick up
> this patch, play around with it and send separately.
On second thought, do you have a link to the last submitted version,
and do you remember if there were any important changes since then?
The base for that one should be closer to "master", I think.
^ permalink raw reply
* Re: [PATCH] rebase --fix: interactive fixup mode
From: Jonathan Nieder @ 2012-01-08 22:01 UTC (permalink / raw)
To: Clemens Buchacher
Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120108213134.GA18671@ecki.lan>
Clemens Buchacher wrote:
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -332,6 +332,15 @@ link:howto/revert-a-faulty-merge.txt[revert-a-faulty-merge How-To] for details).
> user edit that list before rebasing. This mode can also be used to
> split commits (see SPLITTING COMMITS below).
>
> +--fix=<n>::
> + Searches commit history backwards from the current commit until the
> + most recent merge commit, or until a maximum of <n> preceding commits
> + (default: 20), and runs rebase -i <commit>^. The resulting range is
> + typically large enough to contain recent commits which the user might
> + want to edit, while avoiding the usually undesirable effects of
> + rebasing a merge commit, which obviates the need to find a suitable
> + base commit manually.
Funny. :) I wonder if this is possible to generalize, to something like
git rebase -i foo^{last-merge}
or even something like
git rebase -i foo^{first:--merges}
(where "<commit>^{first:<rev-list args>}" would mean something like
"the first commit listed by "git rev-list <rev-list args> <commit>").
What do you think?
^ permalink raw reply
* Re: [PATCH] rebase --fix: interactive fixup mode
From: Jakub Narebski @ 2012-01-08 21:57 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Junio C Hamano
In-Reply-To: <20120108213134.GA18671@ecki.lan>
Clemens Buchacher <drizzd@aon.at> writes:
[...]
> In order to determine a suitable range automatically, it is a reasonable
> heuristic to rebase onto the most recent merge commit.
Why not additionally / instead take into account remote-tracking
branches for "push" remotes?
> It does not
> guarantee that published commits are not included -- indeed there is no
> way to do that. But, the range is usually large enough to contain the
> desired commits. Also, this mechanism works regardless of whether or not
> branch tracking has been configured.
>
> So instead of the above command, one can instead use the following:
>
> git rebase --fix
>
> By default, the range is limited to a maximum of 20 commits. This can be
> changed by passing a different number to --fix, e.g.:
>
> git rebase --fix=50
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Nice idea!
--
Jakub Narebski
Poland
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox