Git development
 help / color / mirror / Atom feed
* Re: [PATCH] rebase --fix: interactive fixup mode
From: Nguyen Thai Ngoc Duy @ 2012-01-09  1:44 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Clemens Buchacher, git, Junio C Hamano
In-Reply-To: <20120108220127.GA4050@burratino>

2012/1/9 Jonathan Nieder <jrnieder@gmail.com>:
> 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?

Is something like this over-generalized?

http://kerneltrap.org/mailarchive/git/2010/12/24/47502

A good thing I see from having a specific option for "-i HEAD~n" is
that it's potentially shorter to type. For someone who does rebase a
lot and has CapsLock turned to Ctrl, it helps. Maybe "rebase -I" ==
"rebase -i HEAD^{last-merge}" (or "rebase -i
<the-revision-used-last-time>") and "rebase -I <n>" == "rebase -i
HEAD~<n>"?
-- 
Duy

^ permalink raw reply

* Re: Aborting "git commit --interactive" discards updates to index
From: Junio C Hamano @ 2012-01-09  0:41 UTC (permalink / raw)
  To: demerphq; +Cc: git, Ævar Arnfjörð
In-Reply-To: <CANgJU+WGEBMMQzsGyQSnMBK3Q8Z2XZdbDx4nr-tB-s0uYEU9CQ@mail.gmail.com>

demerphq <demerphq@gmail.com> writes:

> On 7 January 2012 06:08, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> You are welcome to rehash the age old discussion, though. Personally I do
>> not care very deeply either way. I would never use "commit --interactive"
>> myself, and I would not encourage others to use it, either, even if we do
>> not worry about the behaviour when a commit is aborted.
>
> If I were to provide a patch to make this behavior configurable would
> you have any objections?

You are welcome to rehash the discussion.

I am not a dictator and will listen to other people on the list for their
opinions, and I cannot say if such a patch will be accepted or not without
seeing how well it is done.

>> ... off to run "git add -i" to prepare the index, "git stash save -k" to check
>> out what is to be committed (and stash away what are to be left out) so
>> that you can make sure what you are committing is what you thought are
>> committing (by asking "git diff" and "make test" for example), and after
>
> Isnt this what the diff option in commit interactive is for?

Not at all.

That is to help the user incrementally in the process and not a
replacement for the final eyeballing of the result.

Neither the patch shown in "commit -v", whose primary purpose is to aid
the user to write a better log message.

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Abscissa @ 2012-01-08 23:38 UTC (permalink / raw)
  To: git
In-Reply-To: <1326061722334-7165979.post@n2.nabble.com>

Damn. That 'git-svn-migrate' tool is just giving me errors now. It just gets
through little over 10% of the revisions, and gives:

-----------------------------
Failed to strip path 'bin/lang/.gitignore' ((?-xism:^trunk(/|$)))

- Converting svn:ignore properties into a .gitignore file...
fatal: ambiguous argument 'HEAD': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
log --no-color --no-decorate --first-parent --pretty=medium HEAD: command
returned error: 128
-----------------------------

And then it keeps going and gives:

-----------------------------
To /var/git/Goldie.git
 ! [rejected]        master -> trunk (non-fast-forward)
error: failed to push some refs to '/var/git/Goldie.git'
To prevent you from losing history, non-fast-forward updates were rejected
-----------------------------

I guess I'll have to try some other approach. :/


--
View this message in context: http://git.661346.n2.nabble.com/SVN-Git-but-with-special-changes-tp6840904p7166084.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* Re: [PATCH] rebase --fix: interactive fixup mode
From: Junio C Hamano @ 2012-01-08 22:58 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git
In-Reply-To: <20120108213134.GA18671@ecki.lan>

Clemens Buchacher <drizzd@aon.at> writes:

> 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.

I understand the problem you are trying to solve, but I am not sure if
this is a good idea from the UI point of view for two reasons.

 - "We want to limit the extent of the operation to commits since the last
   merge" is by itself a reasonable thing to ask, and I do not think it
   should be limited to "rebase". If we had an extended SHA-1 syntax to
   express it, for example, you may want to say "I want to see what I did
   since the last merge" and run "git log $last_merge_before_HEAD..".
   Perhaps HEAD~{merge} or something?

 - If your "rebase --fix" is to "fix" things, what is "rebase -i" about?
   Isn't it also about fixing? I imagine that ordinary people expect a
   "fix" option that takes a parameter would take a commit to be fixed,
   and drive the rebase machinery to quickly fix it; in other words,

	$ git rebase --fix=':/^reword the greeting message'

   may internally run "rebase -i $(git rev-parse ':/^rewor...')^", with
   the insn sheet already prepared to "edit" the first commit in it, and
   may even return the control back to the user without showing the insn
   sheet in the editor.

Hmm?

   

^ permalink raw reply

* Re: [PATCH] rebase --fix: interactive fixup mode
From: Clemens Buchacher @ 2012-01-08 22:25 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120108220127.GA4050@burratino>

On Sun, Jan 08, 2012 at 04:01:27PM -0600, Jonathan Nieder wrote:
> 
> Funny. :)  I wonder if this is possible to generalize, to something like
> 
> 	git rebase -i foo^{last-merge}
> 
> What do you think?

I suppose if the history has no merges, I would return the root commit?

Uh, and now I realize I have a bug. In a repo with only linear history:

$ git rebase --fix
fatal: ambiguous argument '4ccf67b9fa2ae247e55b86648d650cb368f286c2^': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
fatal: Needed a single revision
invalid upstream 4ccf67b9fa2ae247e55b86648d650cb368f286c2^

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Abscissa @ 2012-01-08 22:28 UTC (permalink / raw)
  To: git
In-Reply-To: <20120108120807.GA7360@angband.pl>

I see. That's strange, I've been using "apt-get upgrade xxx" for at least a
couple years, I can't believe I never knew that! Thanks for the detailed
info, everyone (and for the patience!)

In answer to someone's question, yea, it's Ubuntu (Kubuntu 10.04, and yea, I
know that's old, but it's not my primary system and I haven't had a chance
yet to upgrade it and get everything set back up again). I also tried it on
a Debian 6 Live/Persistent system and got the same results...apparently for
the same reason.

I did manage to get 1.7.8 installed (on both systems) by downloading and
installing the source (it was kind of a pain figuring out the names of some
of the dependent packages, but I managed to get it.)

So hopefully this should all work now...


--
View this message in context: http://git.661346.n2.nabble.com/SVN-Git-but-with-special-changes-tp6840904p7165979.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* Re: [PATCH] rebase --fix: interactive fixup mode
From: Clemens Buchacher @ 2012-01-08 22:19 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git, Junio C Hamano
In-Reply-To: <m3r4z9eu36.fsf@localhost.localdomain>

On Sun, Jan 08, 2012 at 01:57:03PM -0800, Jakub Narebski wrote:
> 
> > 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?

For me personally, remote-tracking does not work. I frequently branch
locally, and even if I do branch from a remote branch, it's often not
from a public branch, but rather my own private branch that I
synchronize between repos and machines. So my remote-tracking
configuration is usually an awful mess, and it does not feel like fixing
it up manually would be worth the trouble.

As a result, I don't trust remote-tracking and I do not use any of the
features associated with it. For my uses of rebase --fix it would
therefore be counter-productive to consider remote-tracking information.

What I did consider was adding a comment to the list of "pick <commit>"
that interactive rebase offers saying:

# older commits are already contained in the current upstream branch

Also, I often rewrite commits that are also contained in other branches.
That typically happens when I am reworking a topic that has already been
tested extensively. In that case I like to keep the original branch
around for reference, even if I end up not using it eventually.

^ 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

* 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 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

* [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: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

* 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

* [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

* [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 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

* 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

* 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

* 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

* [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 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

* 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 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 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 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


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox