* Re: Cherry picking instead of merges.
From: Björn Steinbrink @ 2008-07-04 0:10 UTC (permalink / raw)
To: David Brown; +Cc: Linus Torvalds, git
In-Reply-To: <20080703223949.GA23092@old.davidb.org>
On 2008.07.03 15:39:49 -0700, David Brown wrote:
> On Thu, Jul 03, 2008 at 02:18:53PM -0700, Linus Torvalds wrote:
>
>> End result: you have a nice merge with nice history that actually
>> converges at a common point, but you effectively did the merge
>> resolution one commit at a time with cherry-picking (or "git rebase",
>> which is obviously just a convenient shorthand for cherry-picking
>> everything).
>
> I'm still not clear how the one-commit-at-a-time resolution gets
> recorded anywhere (except in the cherry-picking branch).
They don't get recorded. Git does not store how merges happened in some
diffs or whatever, it just stores the end result. So what Linus
suggested was to reuse the end result that you already have to create
the merge commit.
Say you have 5 commits like this:
A---B---C
\
D---E
Now there are two ways to get the changes from D and E ontop of C. The
first one is cherry-picking, which leads to:
A---B---C---D'---E'
\
D---E
The other one is merging:
A---B---C---M
\ /
D---E---/
Of course, you should end up with the same tree either way. It's just a
different way of getting towards that final state. So commit E' and
commit M, while different commits, would point to the same tree object.
Now if you want to create that merge commit M, and already have E' (like
you do, as you already did all those cherry-picks), you can use what
Linus suggested. You start the merge, ignore any conflicts and just tell
git to use the tree that E' is pointing to instead.
Björn
^ permalink raw reply
* Re: Question about git-merge-stupid
From: Miklos Vajna @ 2008-07-04 0:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git, Johannes Schindelin
In-Reply-To: <7v4p76y154.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 310 bytes --]
On Thu, Jul 03, 2008 at 04:54:31PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> It is called stupid for a reason ;-).
>
> It has been sitting there as an example for a long time, and I do
> not think anybody minds removing it.
OK, then should I resend a patch that moves it to contrib/examples?
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: "make test" works again (sort-of) on cygwin.
From: Johannes Schindelin @ 2008-07-04 0:04 UTC (permalink / raw)
To: Ramsay Jones; +Cc: GIT Mailing-list, Junio C Hamano
In-Reply-To: <486D0FFC.5090308@ramsay1.demon.co.uk>
Hi,
On Thu, 3 Jul 2008, Ramsay Jones wrote:
> Hi *,
>
> Having upgraded to version 1.5.6 (via tarball), I can "make test" once
> again, without crashing my machine.
Earlier, I would have been more than ready to suggest using msysGit. But
as I became more than just frustrated with "users" (after all, Git users
are supposed to be developers), I refrain more and more from doing so.
Having said that, you _might_ get lucky trying msysGit, even from Cygwin.
Just do not complain to me when it does not work.
Ciao,
Dscho
P.S.: if it does not work, and you fix a few issues, but have problems
with others, I might be more inclined to help you, just because you did
fix some issues.
^ permalink raw reply
* Re: [PATCH 03/15] manpages: fix bogus whitespace
From: Junio C Hamano @ 2008-07-03 23:55 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Christian Couder, J. Bruce Fields, Miklos Vajna,
Shawn O. Pearce
In-Reply-To: <Pine.GSO.4.62.0807022354520.16085@harper.uchicago.edu>
Jonathan Nieder <jrnieder@uchicago.edu> writes:
> It's distracting.
>
> Signed-off-by: Jonathan Nieder <jrnieder@uchicago.edu>
> ---
> Documentation/git-fast-import.txt | 2 +-
> Documentation/git-format-patch.txt | 8 ++++----
> Documentation/git-gui.txt | 4 ++--
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 70cc8e8..9602764 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -86,7 +86,7 @@ OPTIONS
>
> --quiet::
> Disable all non-fatal output, making fast-import silent when it
> - is successful. This option disables the output shown by
> + is successful. This option disables the output shown by
How did you find *this* one? It took me a few minutes to finally notice
that you are talking about the HT. In other words, I had to work hard to
get distracted by it.
^ permalink raw reply
* Re: Question about git-merge-stupid
From: Junio C Hamano @ 2008-07-03 23:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Miklos Vajna, git, Johannes Schindelin
In-Reply-To: <alpine.LFD.1.10.0807030947360.18105@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> Well, see above. I think there's a reason why -stupid isn't even worth
> documenting. It might be better off just removed.
It is called stupid for a reason ;-).
It has been sitting there as an example for a long time, and I do
not think anybody minds removing it.
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Johannes Schindelin @ 2008-07-03 23:53 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Junio C Hamano, git, Daniel Barkalow, Christian Couder
In-Reply-To: <20080703210950.GC6677@leksak.fem-net>
Hi,
On Thu, 3 Jul 2008, Stephan Beyer wrote:
> On Thu, Jul 03, 2008 at 12:03:49PM +0100, Johannes Schindelin wrote:
>
> > On Wed, 2 Jul 2008, Junio C Hamano wrote:
> > > Stephan Beyer <s-beyer@gmx.net> writes:
> > >
> > > > git sequencer is planned as a backend for user scripts that
> > > > execute a sequence of git instructions and perhaps need manual
> > > > intervention, for example git-rebase or git-am.
> > >
> > > ...
> > > > +output () {
> > > > + case "$VERBOSE" in
> > > > + 0)
> > > > + "$@" >/dev/null
> > > > + ;;
> > > > + 1)
> > > > + output=$("$@" 2>&1 )
> > > > + status=$?
> > > > + test $status -ne 0 && printf '%s\n' "$output"
> > > > + return $status
> > > > + ;;
> > > > + 2)
> > > > + "$@"
> > > > + ;;
> > > > + esac
> > > > +}
> > >
> > > Perhaps misnamed? This feels more like "do" or "perform" or "run".
> >
> > My fault. I like "perform".
>
> Right, the "output" name puts a wrong behavior in mind. "perform" (or
> "do" or "run") won't exactly say that this function "adjusts" the output
> somehow, but I'm nonetheless fine with taking "perform".
Actually, it does not even "adjust" the output. It just performs the
action, and _if_ there was an error and "verbose" was set to 1, it shows
the output. If "verbose" was set to 2, it always shows the output.
Otherwise, the output is suppressed.
So, the use of the function really is in the output, but that is either
shown or not shown, never modified.
> Btw, another root commit problem is btw that it's not possible to
> cherry-pick root commits.
That is a problem to be fixed in cherry-pick, not in sequencer. Care to
take care of that?
> Johannes Schindelin wrote:
> > > > +# Usage: pick_one (cherry-pick|revert) [-*|--edit] sha1
> > > > +pick_one () {
> > > > + what="$1"
> > > > + # we just assume that this is either cherry-pick or revert
> > > > + shift
> > > > +
> > > > + # check for fast-forward if no options are given
> > > > + if expr "x$1" : 'x[^-]' >/dev/null
> > > > + then
> > > > + test "$(git rev-parse --verify "$1^")" = \
> > > > + "$(git rev-parse --verify HEAD)" &&
> > > > + output git reset --hard "$1" &&
> > > > + return
> > > > + fi
> > > > + test "$1" != '--edit' -a "$what" = 'revert' &&
> > > > + what='revert --no-edit'
> > >
> > > This looks somewhat wrong.
> > >
> > > When the history looks like ---A---B and we are at A, cherry-picking B can
> > > be optimized to just advancing to B, but that optimization has a slight
> > > difference (or two) in the semantics.
> > >
> > > (1) The committer information would not record the user and time of the
> > > sequencer operation, which actually may be a good thing.
> >
> > This is debatable. But I think you are correct, for all the same reasons
> > why a merge can result in a fast-forward.
>
> Dscho, you mean me by referring to 'you' here, right?
Nope.
> Otherwise I'm a bit confused: "For the same reasons why a merge can
> result in a fast-forward we should not do fast forward here" ;-)
What I meant: there is no use here to redo it. It has already be done,
and redoing just pretends that the girl calling sequencer tried to pretend
that she did it.
If the merge has been done already, it should not be redone.
Only if the user _explicitely_ specified a merge strategy, there _might_
be a reason to redo the merge, but I still doubt it.
> > > (2) When $what is revert, this codepath shouldn't be exercised,
> > > should it?
> >
> > Yes.
>
> I haven't done a check intentionally, but there was a stupid thinko.
> So you're right.
>
> But: this will only be a bug if the commit that _comes next in the
> original history_ is to be reverted.
Does not matter. It's a bug.
A bug is almost always in the details, a corner-case, but it almost always
needs fixing nevertheless.
> Nonetheless, purely tested:
"Nevertheless", maybe? "untested", maybe?
> Johannes Schindelin wrote:
> > I'd not check in sequencer for the strategy. Especially given that we
> > want to support user-written strategies in the future.
>
> I don't know how this is planned to look like, but perhaps
> --list-strategies may make sense here, too.
No. You just do not check for strategies. Period. git-merge does that,
and you can easily abort a rebase if you explicitely asked for an invalid
strategy.
BTW your coalescing multiple replies into one mail was a major pain in the
donkey. Should you do that again, I will not even bother reading that
mail.
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
From: Jakub Narebski @ 2008-07-03 23:45 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, Petr Baudis
In-Reply-To: <486D36CB.3090400@gmail.com>
Lea Wiemann wrote:
> Jakub Narebski wrote:
>>
>> Add get_config([PREFIX]) method [...]
>
> FWIW, I don't think it'll make much of a difference for gitweb, since
> the 'git config -l' output is cached anyway, but it's good someone's
> extracting this. Do you have any user for that function besides gitweb?
If I remember correctly git-cvsserver used to have Perl parser for
_simplified_ config format, but now it uses `git config -l -z` or
`git config -l`. Other git commands written in Perl which uses a lot
of configuration option could use it, like git-svn or git-send-email,
although for them shaving a little bit of execution time is not that
important (git-svn from what I understand still calls git-config
for each config variable).
>> * Should config_val_to_bool and config_val_to_int throw error or
>> just return 'undef' on invalid values?
>
> I suspect that if you have, say, command line tools, throwing an error
> is better UI behavior than silently ignoring the entry. And developers
> can always catch errors if they want to.
Actually for ->config_bool(<VARIABLE>) throwing error was the _only
way_ to distinguish between *non-existent* config variable (which we
can test using exists($config{<VARIABLE>}) when using ->get_config(),
and for which ->config_bool(<VARIABLE>) returned 'undef'), and
*non-bolean* value (for which config_val_to_bool($config{<VARIABLE>})
can return 'undef', and for which ->config_bool(<VARIABLE>) threw
error).
So we don't _need_ to _throw_ an error; we can detect error condition
in other way.
>> * Is "return wantarray ? %config : \%config;" DWIM-mery good style?
>
> Gitweb uses it as well, and it seems reasonable IMVHO.
Errr... if I remember correctly, the code in gitweb is by yours truly
:-), and as I have stated I am *not* a Perl hacker.
>> * Should ->get_config() use ->command_output_pipe, or simpler
>> ->command() method, reading whole config into array?
>
> Does it make a difference? If you're worried about performance, config
> files are so short that it won't matter; use the easier path.
I think using ->command() would be easier...
>> * What should ->get_config() method be named? ->get_config()
>> or perhaps ->config_hash(), or ->config_hashref()?
>
> Regarding the method naming, how about making this an object oriented
> interface? [...] if you can wait a week or so, you could maybe
> integrate this into the Git::Repo interface [...]
I'd rather have both functional (of sorts) and object interface.
Git::Repo / Git::Config could use methods / subroutines from Git.pm;
>> * What should ->get_config() have as an optional parameter:
>> PREFIX (/^$prefix/o), or simply SECTION (/^(?:$section)\./o)?
>
> Off the top of my head, I don't see much need for a prefix parameter, so
> I'd go for 'section'.
O.K. (that is by the way how it is done in gitweb).
> I haven't been able to answer all of the questions, but I hope this helps.
Thanks a lot!
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH/v2] git-basis, a script to manage bases for git-bundle
From: Adam Brewster @ 2008-07-03 23:38 UTC (permalink / raw)
To: git; +Cc: Mark Levedahl, Junio C Hamano, Jakub Narebski
In-Reply-To: <20080703195915.GA18532@sigill.intra.peff.net>
>
> Yes, certainly it is more flexible to have them split. I find Adam's
> argument the most compelling, though. Think about moving commits as a
> multi-step protocol:
>
> 1. Local -> Remote: Here are some new commits, basis..current
> 2. Remote -> Local: OK, I am now at current.
> 3. Local: update basis to current
>
> git-push has the luxury of asking for "basis" each time, so we know it
> is correct. But with bundles, we can't do that. And failing to update
> "basis" means we will send some extra commits next time. But updating
> "basis" when we shouldn't means that the next bundle will be broken.
>
> So I think even if people _do_ want to update "basis" when they create
> the bundle (because it is more convenient, and they are willing to
> accept the possibility of losing sync), it is trivial to create that
> workflow on top of the separate components. But I can see why somebody
> might prefer the separate components, and it is hard to create them if
> the feature is lumped into "git-bundle" (meaning in such a way that you
> cannot perform the steps separately; obviously git-bundle --basis would
> be equivalent).
>
> But I am not a bundle user, so that is just my outsider perspective.
>
> -Peff
>
How does everybody feel about the following:
- Leave git-basis as a small perl script.
- Add a -b/--basis option in git-bundle that calls git-basis. Any
objects mentioned in the output would be excluded from the bundle.
Multiple --basis options will call git-basis once with several
arguments to generate the intersection of specified bases.
- (maybe) Add an option "--update-bases" to automatically call
git-basis --update after the bundle is created successfully.
- Change the syntax a bit so git-basis --show does what git-basis
alone does now (because the user will no longer need to interact with
that command).
There's still plenty of potential for improvements, like a --gc mode
to clean up basis files, a --rewind option to undo an incorrect
--update, or improvements in the way it calculates intersections, but
I think that with these changes the system is as simple as possible
while maximizing flexibility, utility, and usability.
Adam
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Stephan Beyer @ 2008-07-03 23:33 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin
Cc: git, Daniel Barkalow, Christian Couder
In-Reply-To: <7vlk0iy5we.fsf@gitster.siamese.dyndns.org>
Hi again,
On Thu, Jul 03, 2008 at 03:11:45PM -0700, Junio C Hamano wrote:
> Stephan Beyer <s-beyer@gmx.net> writes:
> > + sed -e 's/[$"\\]/\\&/g' -n -e '
> > s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
> > s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
> > s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
> > ###
> >
> > Is escaping $, " and \ enough?
>
> Look at how it is done in git-sh-setup get_author_ident_from_commit.
Yes, single quotes are the other variant and perhaps a little more
robust.
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -520,8 +520,9 @@ clean_author_script () {
# Take "Name <e-mail>" in stdin and outputs author script
make_author_script_from_string () {
- sed -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME="\1"\
-GIT_AUTHOR_EMAIL="\2"\
+ sed -e "s/'/'"'\\'"''/g" \
+ -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME='\''\1'\''\
+GIT_AUTHOR_EMAIL='\''\2'\''\
GIT_AUTHOR_DATE=/'
}
@@ -779,10 +780,10 @@ insn_patch () {
if test -z "$AUTHOR"
then
- sed -n -e '
- s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
- s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
- s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
+ sed -e "s/'/'"'\\'"''/g" -n -e '
+ s/^Author: \(.*\)$/GIT_AUTHOR_NAME='\''\1'\''/p;
+ s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL='\''\1'\''/p;
+ s/^Date: \(.*\)$/GIT_AUTHOR_DATE='\''\1'\''/p
' <"$infofile" >>"$AUTHOR_SCRIPT"
# If sed's result is empty, we keep the original
# author script by appending.
###
On Thu, Jul 03, 2008 at 11:09:50PM +0200, Stephan Beyer wrote:
> And as long as nobody is named $(rm -rf "$HOME") [1], I thought this is
> sufficient for the prototype.
Btw, the [1] should have been a link to http://xkcd.com/327/
Just if you're wondering ;-)
Regards,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* [RFC/PATCH] Remove 'stupid' merge strategy.
From: Miklos Vajna @ 2008-07-03 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Schindelin, git
In-Reply-To: <alpine.LFD.1.10.0807030947360.18105@woody.linux-foundation.org>
As pointed out by Linus, this strategy tries to take the best merge
base, but 'recursive' just does it better. If one needs something more
than 'resolve' then he/she should really use 'recursive' and not
'stupid'.
---
On Thu, Jul 03, 2008 at 10:08:54AM -0700, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> I think -stupid should probably be removed.
Here is an attempt to do so.
> The history of -stupid is from doing the simple single-tree resolve
> that
> git-read-tree can do, but then doing the obvious hack of just trying
> to
> pick the base that gives the least number of conflicts.
>
> HOWEVER.
>
> (...)
Thanks for the detailed answer.
.gitignore | 1 -
Makefile | 3 +-
git-merge-stupid.sh | 80 ---------------------------------------------------
3 files changed, 1 insertions(+), 83 deletions(-)
delete mode 100755 git-merge-stupid.sh
diff --git a/.gitignore b/.gitignore
index 4ff2fec..8054d9d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -75,7 +75,6 @@ git-merge-one-file
git-merge-ours
git-merge-recursive
git-merge-resolve
-git-merge-stupid
git-merge-subtree
git-mergetool
git-mktag
diff --git a/Makefile b/Makefile
index 78e08d3..bddd1a7 100644
--- a/Makefile
+++ b/Makefile
@@ -241,7 +241,6 @@ SCRIPT_SH += git-merge-octopus.sh
SCRIPT_SH += git-merge-one-file.sh
SCRIPT_SH += git-merge-resolve.sh
SCRIPT_SH += git-merge.sh
-SCRIPT_SH += git-merge-stupid.sh
SCRIPT_SH += git-mergetool.sh
SCRIPT_SH += git-parse-remote.sh
SCRIPT_SH += git-pull.sh
@@ -1429,7 +1428,7 @@ check-docs::
do \
case "$$v" in \
git-merge-octopus | git-merge-ours | git-merge-recursive | \
- git-merge-resolve | git-merge-stupid | git-merge-subtree | \
+ git-merge-resolve | git-merge-subtree | \
git-fsck-objects | git-init-db | \
git-?*--?* ) continue ;; \
esac ; \
diff --git a/git-merge-stupid.sh b/git-merge-stupid.sh
deleted file mode 100755
index f612d47..0000000
--- a/git-merge-stupid.sh
+++ /dev/null
@@ -1,80 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Linus Torvalds
-#
-# Resolve two trees, 'stupid merge'.
-
-# The first parameters up to -- are merge bases; the rest are heads.
-bases= head= remotes= sep_seen=
-for arg
-do
- case ",$sep_seen,$head,$arg," in
- *,--,)
- sep_seen=yes
- ;;
- ,yes,,*)
- head=$arg
- ;;
- ,yes,*)
- remotes="$remotes$arg "
- ;;
- *)
- bases="$bases$arg "
- ;;
- esac
-done
-
-# Give up if we are given two or more remotes -- not handling octopus.
-case "$remotes" in
-?*' '?*)
- exit 2 ;;
-esac
-
-# Find an optimum merge base if there are more than one candidates.
-case "$bases" in
-?*' '?*)
- echo "Trying to find the optimum merge base."
- G=.tmp-index$$
- best=
- best_cnt=-1
- for c in $bases
- do
- rm -f $G
- GIT_INDEX_FILE=$G git read-tree -m $c $head $remotes \
- 2>/dev/null || continue
- # Count the paths that are unmerged.
- cnt=`GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l`
- if test $best_cnt -le 0 -o $cnt -le $best_cnt
- then
- best=$c
- best_cnt=$cnt
- if test "$best_cnt" -eq 0
- then
- # Cannot do any better than all trivial merge.
- break
- fi
- fi
- done
- rm -f $G
- common="$best"
- ;;
-*)
- common="$bases"
- ;;
-esac
-
-git update-index --refresh 2>/dev/null
-git read-tree -u -m $common $head $remotes || exit 2
-echo "Trying simple merge."
-if result_tree=$(git write-tree 2>/dev/null)
-then
- exit 0
-else
- echo "Simple merge failed, trying Automatic merge."
- if git-merge-index -o git-merge-one-file -a
- then
- exit 0
- else
- exit 1
- fi
-fi
--
1.5.6.1
^ permalink raw reply related
* Re: [PATCH/v2] git-basis, a script to manage bases for git-bundle
From: Adam Brewster @ 2008-07-03 23:13 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Junio C Hamano, git, Jakub Narebski
In-Reply-To: <486AC8E0.60002@verizon.net>
Hi Mark,
Thank you for your help, and I'm sorry I didn't get back to you sooner.
>
> I have implemented (in script form) a different approach: basically, I just
> keep a local copy of the refs pushed out via bundle in refs/remotes/*, just
> as for any other remote, and then use those as the basis for later bundles.
> My longer term goal is to integrate this into git push, so that with a
> properly configured remote "git push foo" will create a bundle based upon
> the local knowledge of the remote's basis and update the local copy of the
> refs.
>
> [...]
That's a good way to do things. I tend to like my system better
because it's a little more flexible and it doesn't pollute git-branch
-a and gitk --all, also as I said before I like the bundle creation
and the basis update to be separated.
How do you deal with the case where you want to include remote refs in
the bundle? Don't they get saved as
refs/remotes/remote/remotes/somewhere-else/master?
Adam
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Junio C Hamano @ 2008-07-03 22:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, git, Daniel Barkalow, Christian Couder
In-Reply-To: <alpine.DEB.1.00.0807031142540.9925@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> When the history looks like ---A---B and we are at A, cherry-picking B can
>> be optimized to just advancing to B, but that optimization has a slight
>> difference (or two) in the semantics.
>>
>> (1) The committer information would not record the user and time of the
>> sequencer operation, which actually may be a good thing.
>
> This is debatable. But I think you are correct, for all the same reasons
> why a merge can result in a fast-forward.
>
>> (2) When $what is revert, this codepath shouldn't be exercised, should
>> it?
>
> Yes.
>
>> (3) If B is a merge, even if $what is pick, this codepath shouldn't be
>> exercised, should it?
>
> I think it should, again for the same reason why a merge can result in a
> fast-forward.
Sorry, I disagree. "cherry-pick" when this optimization is not applicable
always creates a single parent commit. If the original history looks like:
D---A---B---C
/
M
and when you are cherry-picking B, the above logic would make the result a
merge if you happen to be at A but if you are working elsewhere (perhaps C
or D, or "rewritten A") the result will become a single-parent commit. I
do not see the justification behind such an unreliable/unpredictable
result, from the end-user's point of view.
I like the check and avoidance of creating a commit that will anyway have
the same parent and the same tree as an _optimization_. Making the result
of "cherry-pick B" a merge or a non-merge however is not an optimization;
it is changing semantics.
^ permalink raw reply
* Re: Cherry picking instead of merges.
From: David Brown @ 2008-07-03 22:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0807031403380.2815@woody.linux-foundation.org>
On Thu, Jul 03, 2008 at 02:18:53PM -0700, Linus Torvalds wrote:
>End result: you have a nice merge with nice history that actually
>converges at a common point, but you effectively did the merge resolution
>one commit at a time with cherry-picking (or "git rebase", which is
>obviously just a convenient shorthand for cherry-picking everything).
I'm still not clear how the one-commit-at-a-time resolution gets recorded
anywhere (except in the cherry-picking branch).
It seems to be that I would need to do multiple merges, one at each point
where there is a conflict that I had to resolved. I would remember this as
I did each cherry picked change, but after the fact, I would have to
compare the cherry picked change with the one it came from, and figure out
where conflicts had to be resolved.
Thanks,
David
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Stephan Beyer @ 2008-07-03 22:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Daniel Barkalow, Christian Couder
In-Reply-To: <7vlk0iy5we.fsf@gitster.siamese.dyndns.org>
On Thu, Jul 03, 2008 at 03:11:45PM -0700, Junio C Hamano wrote:
> Stephan Beyer <s-beyer@gmx.net> writes:
> >> > + die_to_continue 'Patch failed. See the .rej files.'
> >> > + # XXX: We actually needed a git-apply flag that creates
> >> > + # conflict markers and sets the DIFF_STATUS_UNMERGED flag.
> >>
> >> That is what -3way is all about, and this codepath is when the user did
> >> not ask for it, isn't it?
> >
> > Now imagine you apply a patch that cannot be applied 100% cleanly and
> > you don't have the 3-way base in the repo. You know what happens?
>
> Do you think I don't? You can check who invented 3way by running "git
> log" or "git blame" on git-am.sh ;-)
I know you know. The question was just rhetorical.
> I think you misread my "That is what -3way is all about". That remark is
> about the comment you have about "creates conflict markers". The conflict
> markers is only possible because we do 3-way merge when you ran "am -3".
> If you do not have the base object but only a blob and an unapplicable
> patch, you cannot do "here is our change since common ancestor, and here
> is their change the patch wants to make" conflict markers, because you do
> not have the common ancestor.
I didn't misread it.
But it is a wrong implication that you cannot do this when you do not have
a common ancestor.
For example: Even if no context matches (or no context exists), it is
possible to add a conflict marker at the specified line. It can happen
that this will result in crap, but resolving some parts that just look
wrong is easier than applying a patch 100% manually.
I think I'm going to add such a git-apply option after GSoC, if nobody
does this before.
> > Yes, the patch is completly rejected, because apply is atomic.
> > And I think a git-apply option that results in a non-atomic behavior,
> > that creates conflict markers (and no .rej files), would be a great
> > usability feature for the "patch" insn in sequencer.
>
> Yes, I think I already said in the message you are responding to that it
> may make sense to have such an option (but at the same time we should
> remember that nobody asked to add --reject to "git am").
Right, so I removed it ;)
Regards,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Junio C Hamano @ 2008-07-03 22:11 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Johannes Schindelin, git, Daniel Barkalow, Christian Couder
In-Reply-To: <20080703210950.GC6677@leksak.fem-net>
Stephan Beyer <s-beyer@gmx.net> writes:
> + sed -e 's/[$"\\]/\\&/g' -n -e '
> s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
> s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
> s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
> ###
>
> Is escaping $, " and \ enough?
Look at how it is done in git-sh-setup get_author_ident_from_commit.
>> > + if test -n "$failed"
>> > + then
>> > + # XXX: This is just a stupid hack:
>> > + with_author git apply $apply_opts --reject --index "$PATCH"
>>
>> Please don't do this without being asked, if you are planning to use this
>> in "am" when 3-way fallback was not asked. It _may_ make sense to give an
>> option to the users to ask for .rej if they prefer to work that way better
>> than working with 3-way merge fallback, but doing this without being asked
>> is not acceptable.
>
> The --reject was just a mind marker for that what I actually think is
> useful and less annoying than the current behavior:
>
>> > + die_to_continue 'Patch failed. See the .rej files.'
>> > + # XXX: We actually needed a git-apply flag that creates
>> > + # conflict markers and sets the DIFF_STATUS_UNMERGED flag.
>>
>> That is what -3way is all about, and this codepath is when the user did
>> not ask for it, isn't it?
>
> Now imagine you apply a patch that cannot be applied 100% cleanly and
> you don't have the 3-way base in the repo. You know what happens?
Do you think I don't? You can check who invented 3way by running "git
log" or "git blame" on git-am.sh ;-)
I think you misread my "That is what -3way is all about". That remark is
about the comment you have about "creates conflict markers". The conflict
markers is only possible because we do 3-way merge when you ran "am -3".
If you do not have the base object but only a blob and an unapplicable
patch, you cannot do "here is our change since common ancestor, and here
is their change the patch wants to make" conflict markers, because you do
not have the common ancestor.
> Yes, the patch is completly rejected, because apply is atomic.
> And I think a git-apply option that results in a non-atomic behavior,
> that creates conflict markers (and no .rej files), would be a great
> usability feature for the "patch" insn in sequencer.
Yes, I think I already said in the message you are responding to that it
may make sense to have such an option (but at the same time we should
remember that nobody asked to add --reject to "git am").
^ permalink raw reply
* Re: [StGit PATCH 03/14] Write to a stack log when stack is modified
From: Catalin Marinas @ 2008-07-03 22:05 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080701201355.GA19096@diana.vm.bytemark.co.uk>
2008/7/1 Karl Hasselström <kha@treskal.com>:
> On 2008-06-17 17:32:47 +0200, Karl Hasselström wrote:
>
>> On 2008-06-17 15:11:42 +0100, Catalin Marinas wrote:
>>
>> > 2008/6/17 Karl Hasselström <kha@treskal.com>:
>> >
>> > > On 2008-06-17 11:24:53 +0100, Catalin Marinas wrote:
>> > >
>> > > > 2008/6/12 Karl Hasselström <kha@treskal.com>:
>> > > >
>> > > > > class _Directory(object):
>> > > > > - def __init__(self, needs_current_series = True):
>> > > > > + def __init__(self, needs_current_series = True, log = True):
>> > > >
>> > > > i.e. we make log = False here by default.
>> > >
>> > > I might not have understood precisely what you meant; but I
>> > > don't think API backwards compatibilty should be an issue here?
>> > > I simply fix all callers. If log should default to true or false
>> > > is immaterial -- it just means some extra text in one or the
>> > > other of two equally common cases.
>> >
>> > Not an issue, I just favour the existing one when the two cases
>> > are almost equal.
>>
>> Fair enough. I'll change it.
>
> I had an even better idea: no default value. Every caller gets to say
> either log = True or log = False, which makes it immediately obvious
> to the reader. (That is, every caller still using the old
> infrastructure; with the new infrastructure, we log if and only if a
> transaction is run.)
Fair enough.
--
Catalin
^ permalink raw reply
* Re: Can I remove stg sync --undo ?
From: Catalin Marinas @ 2008-07-03 22:02 UTC (permalink / raw)
To: Karl Hasselström; +Cc: git
In-Reply-To: <20080702072524.GA26805@diana.vm.bytemark.co.uk>
Hi Karl,
Sorry for the delay, I've been really busy recently.
2008/7/2 Karl Hasselström <kha@treskal.com>:
> I'm preparing a patch that removes all the old --undo flags, and
> discovered that stg sync has an --undo flag backed by
> stack.undo_refresh().
The current --undo flag restores the state of the patch before a
successful sync. If the sync fails with a conflict and it needs a
refresh after resolving, I think it loses the previous state of the
patch and just restores to whatever it was before the refresh.
> Is it OK if I remove it? "stg undo" will allow you to undo the whole
> command, or, in case of conflicts, either the whole command or just
> the last conflicting push. But it does not allow for undoing the last
> refresh (whether it succeeded or not). I don't know how refresh is
> used, so I can't really tell if "stg undo" is currently insufficient.
The sync performs three operations - push, merge and refresh (if the
refresh is automatic after merge, it doesn't update the backup
information since it was done by merge).
If merge fails, the refresh is manual after solving the conflicts. I
suspect this will be recorded as a separate step for undo (BTW, is
resolved take into account for undo?).
--
Catalin
^ permalink raw reply
* Re: [PATCH 2/3] git-add--interactive: remove hunk coalescing
From: Junio C Hamano @ 2008-07-03 21:46 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <200807032124.22037.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Junio C Hamano wrote:
>>
>> > Blindly concatenating the above two and feeding them to "git apply" *may*
>> > happen to work by accident, not by design. This very much feels like a
>> > hack of "This works most of the time for me, your mileage may vary" kind,
>> > which we would want to avoid when we can.
>>
>> Well, I changed my mind. Let's run with this and see what happens.
>
> In support of this being a feature of git-apply, notice that it even
> handles the situation correctly where the context of a hunk has been
> influenced by previous hunks, as in...
That's what meant by my earlier "application is hunk-by-hunk in nature"
and we are in agreement. The fact it works that way is not quite by
design and close to being "by accident", but I do not foresee anybody
changing it in the near future, so...
^ permalink raw reply
* Re: Patches for qgit on MacOS X
From: Marco Costalba @ 2008-07-03 21:46 UTC (permalink / raw)
To: Olivier Croquette; +Cc: git
In-Reply-To: <4869E55C.2020608@free.fr>
On Tue, Jul 1, 2008 at 6:05 PM, Olivier Croquette <ocroquette@free.fr> wrote:
> Marco Costalba wrote, On 30/06/08 12:00:
>>
>> Thanks for the patches, I'll apply them.
>
> Great!
>
>> Can I ask why don't you use qgit-2.X series ?
>
> Sure you can, it is just because I overlooked the existence of the
> version 2 :)
>
> After compiling qt4-mac from source (it took quite some time), I have been
> able to compile qgit2 too without a hitch, so I guess the patch is
> irrelevant for qgit2. Are you using a different mechanism to access the
> environment?
>
Sorry for my late reply, but I'm traveling these days.
No I don't use a different mechanism I always access the environment
through the Qt libraries, so probably the improvment is in Qt4 over
Qt3.
Marco
^ permalink raw reply
* [StGit PATCH v2] Reuse the same temp index in a transaction
From: Karl Hasselström @ 2008-07-03 21:38 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
In-Reply-To: <20080702061314.11361.28297.stgit@yoghurt>
Instead of making a new temp index every time we need one, just keep
reusing the same one. And keep track of which tree is currently stored
in it -- if we do several consecutive successful pushes, it's always
going to be the "right" tree so that we don't have to call read-tree
before each patch application.
The motivation behind this change is of course that it makes things
faster.
(The same simple test as in the previous patch -- pushing 250 patches
in a 32k-file repository, with one file-level merge necessary per push
-- went from 0.36 to 0.19 seconds per patch with this patch applied.)
Signed-off-by: Karl Hasselström <kha@treskal.com>
---
Here's the same patch, but with Repository.index_merge() instead
called Index.merge(). A much better fit, I think.
Also some doc changes compared to v1.
stgit/lib/git.py | 63 +++++++++++++++++++++++++++++++---------------
stgit/lib/transaction.py | 12 ++++++++-
2 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 9402606..8fb8bd2 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -459,31 +459,12 @@ class Repository(RunWithEnv):
def set_head_ref(self, ref, msg):
self.run(['git', 'symbolic-ref', '-m', msg, 'HEAD', ref]).no_output()
def simple_merge(self, base, ours, theirs):
- """Given three L{Tree}s, tries to do an in-index merge with a
- temporary index. Returns the result L{Tree}, or None if the
- merge failed (due to conflicts)."""
- assert isinstance(base, Tree)
- assert isinstance(ours, Tree)
- assert isinstance(theirs, Tree)
-
- # Take care of the really trivial cases.
- if base == ours:
- return theirs
- if base == theirs:
- return ours
- if ours == theirs:
- return ours
-
index = self.temp_index()
- index.read_tree(ours)
try:
- try:
- index.apply_treediff(base, theirs)
- return index.write_tree()
- except MergeException:
- return None
+ result, index_tree = index.merge(base, ours, theirs)
finally:
index.delete()
+ return result
def apply(self, tree, patch_text):
"""Given a L{Tree} and a patch, will either return the new L{Tree}
that results when the patch is applied, or None if the patch
@@ -563,6 +544,46 @@ class Index(RunWithEnv):
# contains all involved objects; in other words, we don't have
# to use --binary.
self.apply(self.__repository.diff_tree(tree1, tree2, ['--full-index']))
+ def merge(self, base, ours, theirs, current = None):
+ """Use the index (and only the index) to do a 3-way merge of the
+ L{Tree}s C{base}, C{ours} and C{theirs}. The merge will either
+ succeed (in which case the first half of the return value is
+ the resulting tree) or fail cleanly (in which case the first
+ half of the return value is C{None}).
+
+ If C{current} is given (and not C{None}), it is assumed to be
+ the L{Tree} currently stored in the index; this information is
+ used to avoid having to read the right tree (either of C{ours}
+ and C{theirs}) into the index if it's already there. The
+ second half of the return value is the tree now stored in the
+ index, or C{None} if unknown. If the merge succeeded, this is
+ often the merge result."""
+ assert isinstance(base, Tree)
+ assert isinstance(ours, Tree)
+ assert isinstance(theirs, Tree)
+ assert current == None or isinstance(current, Tree)
+
+ # Take care of the really trivial cases.
+ if base == ours:
+ return (theirs, current)
+ if base == theirs:
+ return (ours, current)
+ if ours == theirs:
+ return (ours, current)
+
+ if current == theirs:
+ # Swap the trees. It doesn't matter since merging is
+ # symmetric, and will allow us to avoid the read_tree()
+ # call below.
+ ours, theirs = theirs, ours
+ if current != ours:
+ self.read_tree(ours)
+ try:
+ self.apply_treediff(base, theirs)
+ result = self.write_tree()
+ return (result, result)
+ except MergeException:
+ return (None, ours)
def delete(self):
if os.path.isfile(self.__filename):
os.remove(self.__filename)
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index e47997e..74bc74d 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -1,6 +1,8 @@
"""The L{StackTransaction} class makes it possible to make complex
updates to an StGit stack in a safe and convenient way."""
+import atexit
+
from stgit import exception, utils
from stgit.utils import any, all
from stgit.out import *
@@ -84,6 +86,7 @@ class StackTransaction(object):
self.__allow_conflicts = lambda trans: allow_conflicts
else:
self.__allow_conflicts = allow_conflicts
+ self.__temp_index = self.temp_index_tree = None
stack = property(lambda self: self.__stack)
patches = property(lambda self: self.__patches)
def __set_applied(self, val):
@@ -97,6 +100,12 @@ class StackTransaction(object):
or self.patches[self.applied[0]].data.parent == val)
self.__base = val
base = property(lambda self: self.__base, __set_base)
+ @property
+ def temp_index(self):
+ if not self.__temp_index:
+ self.__temp_index = self.__stack.repository.temp_index()
+ atexit.register(self.__temp_index.delete)
+ return self.__temp_index
def __checkout(self, tree, iw):
if not self.__stack.head_top_equal():
out.error(
@@ -238,7 +247,8 @@ class StackTransaction(object):
base = oldparent.data.tree
ours = cd.parent.data.tree
theirs = cd.tree
- tree = self.__stack.repository.simple_merge(base, ours, theirs)
+ tree, self.temp_index_tree = self.temp_index.merge(
+ base, ours, theirs, self.temp_index_tree)
merge_conflict = False
if not tree:
if iw == None:
^ permalink raw reply related
* Re: Cherry picking instead of merges.
From: Samuel Tardieu @ 2008-07-03 21:18 UTC (permalink / raw)
To: David Brown; +Cc: Avery Pennarun, git
In-Reply-To: <20080703205329.GA17923@old.davidb.org>
>>>>> "David" == David Brown <git@davidb.org> writes:
David> It's only sitting in a private developer's branch. I want to
David> do the merge properly, but I'm just trying to figure out how to
David> get the conflict resolution out of his work.
Why don't you create a commit which has both your (A) and company B
branch head (B) as parent with the same content as the private
developer's branch (P) which contains the succesful merge?
Something like that: (totally untested)
% git checkout A
% git merge --no-commit -s ours B
% git checkout P .
% git commit -a
^ permalink raw reply
* Re: ':/<oneline prefix>' notation doesn't support full file syntax
From: Junio C Hamano @ 2008-07-03 21:26 UTC (permalink / raw)
To: Dana How; +Cc: Eric Raible, Junio C Hamano, git, Johannes.Schindelin
In-Reply-To: <56b7f5510807031127j10e33f3bl516180f7a9b5b5db@mail.gmail.com>
"Dana How" <danahow@gmail.com> writes:
> I was surprised to see Dscho advocating removing this feature altogether.
> Others proposed other command sequences which avoided :/ .
> If :/ is now going to be extended and thus perhaps more likely to
> appear in scripts,
> is now the time to change it to ? which has no other special meaning to git?
There are number of problems with ":/" notation, but my biggest gripe is
that it is only slightly better than "give back a random commit". You
cannot even tell it to "dig from these branch tips, look for the first one
that talks about this text".
As Dscho mentioned, --grep works much better and instead of saying:
$ git diff ':/send-email' HEAD
we can say:
$ git diff \
$(git log --pretty=format:%H -1 --grep=send-email master next) HEAD
The error behaviour is somewhat different between the two, though. When
you misspell what to grep, the command substitution will give empty and
you would get an unexpected result. Being built-in, ':/' syntax can say
"I do not find anything that match" fairly easily, and the command
substitution version has to say something ugly like:
$ git diff \
$(
x=$(git log --pretty=format:%H -1 --grep=send-email master next)
case "$x" in
('') echo 0000000000000000000000000000000000000000 ;;
(?) echo $x ;; esac
) HEAD
to get a similar effect.
But the point is that you can extend it easily with the :path suffix if
you wanted to:
$ git show \
$(git log --pretty=format:%H -1 --grep=send-email):git-send-email.perl
You can even alias "log --pretty=format:%H -1" if you wanted to, and use
revision limiter other than --grep, like this:
(in .git/config)
[alias]
pick = log --pretty=format:%H -1
$ git diff --stat $(git pick -- Documentation)^
$ git blame $(git pick pu -- remote.c) remote.c
So in short, ':/' is limited (cannot be suffixed with :path, cannot be
told to dig down from named revs, etc.) but you can do what ':/' cannot do
fairly easily with command substitution.
However, $(git pick --all --grep=something), without suffixed modifiers
such as ~$N and :$path, may still be common enough that it might deserve a
short-hand ':/' (and that is why we have it).
If people do not find that short-hand useful, I am not strongly opposed to
the idea of dropping it. I personally find the notation not very useful
cute hack anyway ;-).
^ permalink raw reply
* Re: Cherry picking instead of merges.
From: Linus Torvalds @ 2008-07-03 21:18 UTC (permalink / raw)
To: David Brown; +Cc: git
In-Reply-To: <20080703182650.GA11166@old.davidb.org>
On Thu, 3 Jul 2008, David Brown wrote:
>
> First we tried a git-merge and resolved the conflicts. The problem here is
> that the resultant code didn't work. git-bisect wasn't very useful because
> the intermediate versions don't have resolved conflicts.
>
> Yesterday, one developer cherry picked company B's changes into a branch.
> It appears he resolved the conflicts for each commit, which should make
> bisecting easier.
What I would suggest is actually a stupid combination of the two. The
advantage and disadvantage of the cherry-picking is:
- the cherry-picking obviously gave you the end result you wanted, and is
bisectable at a per-commit level (ie each cherry-pick was a
"micro-merge", and if there was a merge problem, it would show up
exactly in _that_ one)
BUT
- the cherry-picking doesn't help future merges, since the history is
disjoint, and as such it will always diverge and just cause more and
more problems down the road to keep on doing this.
so what I would suggest is
- generate the *state* of the tree by cherry-picking each commit one by
one, and verify that state.
- but then do a merge that picks that one state as the merge result
(this is all assuming you cherry-picked every single commit, of
course!)
The latter thing is really easy to do. Just do the merge, and ignore the
conflicts that happen entirely, because you then just use the tree from
the temporary cherry-picking branch
# Try to merge the other branch ..
git merge otherbranch
# .. but ignore conflicts entirely by then
# setting the index and checked-out state to
# the known-good end result
git read-tree -u --reset cherry-picked-branch
# .. and commit that instead
git commit
# and you can now remove the cherry-picked branch
# that was only used for the tree
git branch -D cherry-picked-branch
to actually commit that merge and get rid of the temporary cherry-picking
branch.
(Of course, if the merge actually doesn't generate any conflicts, and thus
commits it, but doesn't actually _work_, then that "git commit" needs to
be a "git commit --amend" instead, in order to actually replace the merge
commit rather than add a fixup commit afterwards)
End result: you have a nice merge with nice history that actually
converges at a common point, but you effectively did the merge resolution
one commit at a time with cherry-picking (or "git rebase", which is
obviously just a convenient shorthand for cherry-picking everything).
This actually has a few other advantages too: "git show --cc" on that
merge will also show the conflicts the way they got resolved, so you
actually end up with some useful information this way too.
Linus
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Stephan Beyer @ 2008-07-03 21:12 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <m3d4lvf70w.fsf@localhost.localdomain>
Hi,
first, thanks for the --root idea ;-)
> BTW. what is best way of checking if given revision is parent-less?
I think count the number of parents is quite sane.
In sequencer I have:
count_parents() {
git cat-file commit "$1" | sed -n -e '1,/^$/p' | grep -c '^parent'
}
Regards,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ permalink raw reply
* Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype
From: Stephan Beyer @ 2008-07-03 21:09 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin
Cc: git, Daniel Barkalow, Christian Couder
In-Reply-To: <7vbq1f68rh.fsf@gitster.siamese.dyndns.org>
Hi,
On Thu, Jul 03, 2008 at 12:03:49PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Wed, 2 Jul 2008, Junio C Hamano wrote:
> > Stephan Beyer <s-beyer@gmx.net> writes:
> >
> > > git sequencer is planned as a backend for user scripts
> > > that execute a sequence of git instructions and perhaps
> > > need manual intervention, for example git-rebase or git-am.
> >
> > ...
> > > +output () {
> > > + case "$VERBOSE" in
> > > + 0)
> > > + "$@" >/dev/null
> > > + ;;
> > > + 1)
> > > + output=$("$@" 2>&1 )
> > > + status=$?
> > > + test $status -ne 0 && printf '%s\n' "$output"
> > > + return $status
> > > + ;;
> > > + 2)
> > > + "$@"
> > > + ;;
> > > + esac
> > > +}
> >
> > Perhaps misnamed? This feels more like "do" or "perform" or "run".
>
> My fault. I like "perform".
Right, the "output" name puts a wrong behavior in mind.
"perform" (or "do" or "run") won't exactly say that this function
"adjusts" the output somehow, but I'm nonetheless fine with taking
"perform".
(I inline-attach patches so that you can incrementally review what I did
with your feedback.)
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -75,7 +75,7 @@ quit () {
exit 0
}
-output () {
+perform () {
case "$VERBOSE" in
0)
"$@" >/dev/null
@@ -165,7 +165,7 @@ restore () {
git symbolic-ref HEAD "$HEADNAME"
;;
esac &&
- output git reset --hard "$HEAD"
+ perform git reset --hard "$HEAD"
}
has_action () {
@@ -208,14 +208,14 @@ pick_one () {
then
test "$(git rev-parse --verify "$1^")" = \
"$(git rev-parse --verify HEAD)" &&
- output git reset --hard "$1" &&
+ perform git reset --hard "$1" &&
return
fi
test "$1" != '--edit' -a "$what" = 'revert' &&
what='revert --no-edit'
test -n "$SIGNOFF" &&
what="$what -s"
- $use_output git $what "$@"
+ $use_perform git $what "$@"
}
nth_string () {
@@ -945,15 +945,15 @@ insn_pick () {
# Be kind to users and ignore --mainline=1 on non-merge commits
test -n "$mainline" -a 2 -gt $(count_parents "$sha1") && mainline=
- use_output=
+ use_perform=
test -n "$edit_msg" ||
- use_output=output
+ use_perform=perform
pick_one "$op" $edit_msg $mainline $sha1 ||
die_with_patch $sha1 "Could not apply $sha1"
test -n "$EDIT" ||
- use_output=output
+ use_perform=perform
get_current_author
signoff=
@@ -961,18 +961,18 @@ insn_pick () {
if test -n "$AUTHOR" -a -n "$MESSAGE"
then
# this is just because we only want to do ONE amending commit
- $use_output git commit --amend $EDIT $signoff --no-verify \
+ $use_perform git commit --amend $EDIT $signoff --no-verify \
--author "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" \
--message="$MESSAGE"
else
# correct author if AUTHOR is set
test -n "$AUTHOR" &&
- $use_output git commit --amend $EDIT --no-verify -C HEAD \
+ $use_perform git commit --amend $EDIT --no-verify -C HEAD \
--author "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>"
# XXX: a git-cherry-pick --author could be nicer here
# correct commit message if MESSAGE is set
test -n "$MESSAGE" &&
- $use_output git commit --amend $EDIT $signoff --no-verify \
+ $use_perform git commit --amend $EDIT $signoff --no-verify \
-C HEAD --message="$MESSAGE"
fi
@@ -1128,12 +1128,12 @@ insn_squash () {
case "$(peek_next_command)" in
squash)
edit_commit=
- use_output=output
+ use_perform=perform
cp "$MSG" "$squash_msg"
;;
*)
edit_commit=-e
- use_output=
+ use_perform=
rm -f "$squash_msg" || exit
;;
esac
@@ -1157,9 +1157,9 @@ insn_squash () {
failed=
if test -n "$from"
then
- output git reset --soft "$sha1"
+ perform git reset --soft "$sha1"
else
- output git reset --soft HEAD^
+ perform git reset --soft HEAD^
pick_one cherry-pick -n "$sha1" || failed=t
fi
@@ -1169,7 +1169,7 @@ insn_squash () {
if test -z "$failed"
then
# This is like --amend, but with a different message
- with_author $use_output git commit --no-verify \
+ with_author $use_perform git commit --no-verify \
-F "$MSG" $edit_commit || failed=t
else
cp "$MSG" "$GIT_DIR/MERGE_MSG"
@@ -1320,13 +1320,13 @@ insn_merge () {
fi
mark_action_done
- if ! with_author output git merge $strategy -m junk $new_parents
+ if ! with_author perform git merge $strategy -m junk $new_parents
then
git rerere
cat "$MSG" >"$GIT_DIR/MERGE_MSG"
die_to_continue 'Error merging'
fi
- with_author output git commit --amend -F "$MSG" --no-verify
+ with_author perform git commit --amend -F "$MSG" --no-verify
return 0
}
@@ -1352,7 +1352,7 @@ insn_reset () {
comment_for_reflog reset
mark_action_done
- output git reset --hard "$(mark_to_commit "$1")"
+ perform git reset --hard "$(mark_to_commit "$1")"
}
@@ -1375,7 +1375,7 @@ insn_ref () {
comment_for_reflog ref
mark_action_done
- output git update-ref "$1" HEAD
+ perform git update-ref "$1" HEAD
}
@@ -1547,10 +1547,10 @@ do_startup () {
then
ONTO="refs/heads/${ONTO##*/}"
echo "$ONTO" >"$SEQ_DIR/onto"
- output git checkout "$(git rev-parse "$ONTO")" ||
+ perform git checkout "$(git rev-parse "$ONTO")" ||
die_abort "Could not checkout branch $ONTO"
else
- output git checkout "$ONTO" ||
+ perform git checkout "$ONTO" ||
die_abort "Could not checkout commit $ONTO"
ONTO=
fi
@@ -1653,7 +1653,7 @@ Fix with git sequencer --edit or abort with $(print_caller --abort)."
comment_for_reflog skip
git rerere clear
- output git reset --hard "$(cat "$SEQ_DIR/skiphead")" &&
+ perform git reset --hard "$(cat "$SEQ_DIR/skiphead")" &&
rm -f "$WHY_FILE" &&
echo '# SKIPPED the last instruction' >>"$DONE" &&
execute_rest
###
On Wed, Jul 02, 2008 at 06:45:06PM -0700, Junio C Hamano wrote:
> > +LAST_COUNT=
> > +mark_action_done () {
> > + sed -e 1q <"$TODO" >>"$DONE"
> > + sed -e 1d <"$TODO" >"$TODO.new"
> > + mv -f "$TODO.new" "$TODO"
> > + if test "$VERBOSE" -gt 0
> > + then
> > + count=$(grep -c '^[^#]' <"$DONE")
> > + total=$(expr "$count" + "$(grep -c '^[^#]' <"$TODO")")
>
> Here we are not counting lines that are comments as insns (I am not
> complaining; just making a mental note).
Add to your note, that empty lines are also skipped, because at least
one character (which is not #) must be there.
And add to your note, that leading whitespace is not ignored on that
checks, which can lead to wrong numbers, but: (quoting Dscho)
> "count" and "total" are only used for the progress output
(The builtin handles that better because it just does proper parsing
and not such hacks.)
Junio C Hamano wrote:
> > + if test "$LAST_COUNT" != "$count"
> > + then
> > + LAST_COUNT="$count"
> > + test "$VERBOSE" -lt 1 ||
> > + printf 'Sequencing (%d/%d)\r' "$count" "$total"
> > + test "$VERBOSE" -lt 2 || echo
> > + fi
> > + fi
> > +}
> > +
> > +# Generate message, patch and author script files
> > +make_patch () {
> > + parent_sha1=$(git rev-parse --verify "$1"^) ||
> > + die "Cannot get patch for $1^"
> > + git diff-tree -p "$parent_sha1..$1" >"$PATCH"
>
> Could there be a case where we need/want to deal with a root commit
> without parents?
Yes, and this is not handled correctly.
I've taken Jakub's idea:
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -139,9 +139,9 @@ mark_action_done () {
# Generate message, patch and author script files
make_patch () {
- parent_sha1=$(git rev-parse --verify "$1"^) ||
- die "Cannot get patch for $1^"
- git diff-tree -p "$parent_sha1..$1" >"$PATCH"
+ parent_sha1=$(git rev-parse --verify "$1^" 2>/dev/null ||
+ echo '--root')
+ git diff-tree -p "$parent_sha1" "$1" >"$PATCH"
test -f "$MSG" ||
commit_message "$1" >"$MSG"
test -f "$AUTHOR_SCRIPT" ||
###
Btw, another root commit problem is btw that it's not possible
to cherry-pick root commits.
Junio C Hamano wrote:
> > + test -f "$MSG" ||
> > + commit_message "$1" >"$MSG"
> > + test -f "$AUTHOR_SCRIPT" ||
> > + get_author_ident_from_commit "$1" >"$AUTHOR_SCRIPT"
> > +}
> > +
> > +# Generate a patch and die with "conflict" status code
> > +die_with_patch () {
> > + make_patch "$1"
> > + git rerere
> > + die_to_continue "$2"
> > +}
> > +
> > +restore () {
> > + git rerere clear
> > +
> > + HEADNAME=$(cat "$SEQ_DIR/head-name")
> > + HEAD=$(cat "$SEQ_DIR/head")
>
> Perhaps
>
> read HEADNAME <"$SEQ_DIR/head-name"
>
> provided if these values are $IFS safe?
They are IFS-safe. I don't really have an opinion if
read FOO <foo
is better than
FOO=$(cat foo)
or vice versa, so I have no problem to change this.
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -158,8 +158,8 @@ die_with_patch () {
restore () {
git rerere clear
- HEADNAME=$(cat "$SEQ_DIR/head-name")
- HEAD=$(cat "$SEQ_DIR/head")
+ read HEADNAME <"$SEQ_DIR/head-name"
+ read HEAD <"$SEQ_DIR/head"
case $HEADNAME in
refs/*)
git symbolic-ref HEAD "$HEADNAME"
@@ -1496,9 +1496,10 @@ prepare_editable_todo () {
}
get_saved_options () {
- VERBOSE=$(cat "$SEQ_DIR/verbose")
- ONTO=$(cat "$SEQ_DIR/onto")
- WHY=$(cat "$WHY_FILE" 2>/dev/null)
+ read VERBOSE <"$SEQ_DIR/verbose"
+ read ONTO <"$SEQ_DIR/onto"
+ test -f "$WHY_FILE" &&
+ read WHY <"$WHY_FILE"
return 0
}
###
Johannes Schindelin wrote:
> > > +# Usage: pick_one (cherry-pick|revert) [-*|--edit] sha1
> > > +pick_one () {
> > > + what="$1"
> > > + # we just assume that this is either cherry-pick or revert
> > > + shift
> > > +
> > > + # check for fast-forward if no options are given
> > > + if expr "x$1" : 'x[^-]' >/dev/null
> > > + then
> > > + test "$(git rev-parse --verify "$1^")" = \
> > > + "$(git rev-parse --verify HEAD)" &&
> > > + output git reset --hard "$1" &&
> > > + return
> > > + fi
> > > + test "$1" != '--edit' -a "$what" = 'revert' &&
> > > + what='revert --no-edit'
> >
> > This looks somewhat wrong.
> >
> > When the history looks like ---A---B and we are at A, cherry-picking B can
> > be optimized to just advancing to B, but that optimization has a slight
> > difference (or two) in the semantics.
> >
> > (1) The committer information would not record the user and time of the
> > sequencer operation, which actually may be a good thing.
>
> This is debatable. But I think you are correct, for all the same reasons
> why a merge can result in a fast-forward.
Dscho, you mean me by referring to 'you' here, right?
Otherwise I'm a bit confused: "For the same reasons why a merge can result
in a fast-forward we should not do fast forward here" ;-)
I think the ff is more useful than real picking and I think the rebase-i
test suite even has a test case for this.
(Checked: "* FAIL 17: merge redoes merges" if it is removed, so it's
implicitly checked for redone merges.)
> > (2) When $what is revert, this codepath shouldn't be exercised, should
> > it?
>
> Yes.
I haven't done a check intentionally, but there was a stupid thinko.
So you're right.
But: this will only be a bug if the commit that _comes next in the
original history_ is to be reverted. This is usually user-nonsense
(I can't imagine a useful application for that), but nonetheless a
user can try it for fun and such a case is not worth a sanity check.
So it's perhaps good to add a test here and just do the revert (and no
ff) as the user requests.
> > (3) If B is a merge, even if $what is pick, this codepath shouldn't be
> > exercised, should it?
>
> I think it should, again for the same reason why a merge can result in a
> fast-forward.
Right.
Junio C Hamano wrote:
> As to the syntax I tend to prefer
>
> case "$1" in
> -*) ... do option thing ... ;;
> *) ... do other thing... ;;
> esac
Christian has mentioned this, but I didn't listen to him ;)
Because I think, if you put the cases, the do_thing and the ;;
on separate lines,
1. it is just consistent compared to the cases where you
do several things ... you don't do them on one line and
you put the ;; on an extra line
2. the diffs will look better on extensions
> So how about...
>
> case "$what,$1" in
> revert,--edit)
> what='revert --no-edit' ;;
> revert,* | cherry-pick,-* )
> ;;
> *)
> if ! git rev-parse --verify "$1^2" &&
> test "$(git rev-parse --verify "$1^") = \
> "$(git rev-parse --verify HEAD)"
> then
> output git reset --hard "$1"
> return
> fi
> ;;
> esac
I've done this:
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -200,19 +200,25 @@ with_author () {
# Usage: pick_one (cherry-pick|revert) [-*|--edit] sha1
pick_one () {
what="$1"
- # we just assume that this is either cherry-pick or revert
shift
- # check for fast-forward if no options are given
- if expr "x$1" : 'x[^-]' >/dev/null
- then
- test "$(git rev-parse --verify "$1^")" = \
- "$(git rev-parse --verify HEAD)" &&
- perform git reset --hard "$1" &&
+ case "$what,$1" in
+ revert,*)
+ test "$1" != '--edit' &&
+ what='revert --no-edit'
+ ;;
+ cherry-pick,-*)
+ ;;
+ cherry-pick,*)
+ # fast forward
+ if test "$(git rev-parse --verify "$1^")" = \
+ "$(git rev-parse --verify HEAD)"
+ then
+ perform git reset --hard "$1"
return
- fi
- test "$1" != '--edit' -a "$what" = 'revert' &&
- what='revert --no-edit'
+ fi
+ ;;
+ esac
test -n "$SIGNOFF" &&
what="$what -s"
$use_perform git $what "$@"
###
Johannes Schindelin wrote:
> > > +make_squash_message () {
> > > + if test -f "$squash_msg"
> > > + then
> > > + count=$(($(sed -n -e 's/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p' \
> > > + <"$squash_msg" | sed -n -e '$p')+1))
> > > + echo "# This is a combination of $count commits."
> > > + sed -e '1d' -e '2,/^./{
> > > + /^$/d
> > > + }' <"$squash_msg"
> > > + else
> > > + count=2
> > > + echo '# This is a combination of 2 commits.'
> > > + echo '# The first commit message is:'
> > > + echo
> > > + commit_message HEAD
> > > + fi
> > > + echo
> > > + echo "# This is the $(nth_string "$count") commit message:"
> > > + echo
> > > + commit_message "$1"
> > > +}
> > > +
> > > +make_squash_message_multiple () {
> > > + echo '# This is a dummy to get the 0.' >"$squash_msg"
> > > + for cur_sha1 in $(git rev-list --reverse "$sha1..HEAD")
> > > + do
> > > + make_squash_message "$cur_sha1" >"$MSG"
> > > + cp "$MSG" "$squash_msg"
> > > + done
> > > +}
> >
> > Hmm, I know this is how rebase-i is written, but we should be able to do
> > better than writing and flipping temporary times N times, shouldn't we?
>
> Right, again my fault.
No, not your fault.
I think Junio is referring to make_squash_message_multiple which is for
squash --from <mark>
and just calls your make_squash_message several times.
When I did this, I knew that this is not most efficient, but I had in
mind: this is just a prototype. The builtin will solve this in a
different way.
Nonetheless, purely tested:
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -264,11 +264,26 @@ make_squash_message () {
}
make_squash_message_multiple () {
- echo '# This is a dummy to get the 0.' >"$squash_msg"
- for cur_sha1 in $(git rev-list --reverse "$sha1..HEAD")
+ revlist=$(git rev-list --reverse "$sha1..HEAD")
+ count=$(echo "$revlist" | wc -l)
+ squash_i=0
+ echo "# This is a combination of $count commits."
+ for cur_sha1 in $revlist
do
- make_squash_message "$cur_sha1" >"$MSG"
- cp "$MSG" "$squash_msg"
+ squash_i=$(($squash_i+1))
+ if test -f "$squash_msg"
+ then
+ count=$(($count + $(sed -n -e \
+ 's/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p' \
+ <"$squash_msg" | sed -n -e '$p')+1))
+ sed -e '1d' -e '2,/^./{
+ /^$/d
+ }' <"$squash_msg"
+ fi
+ echo
+ echo "# This is the $(nth_string "$squash_i") commit message:"
+ echo
+ commit_message "$cur_sha1"
done
}
@@ -1128,7 +1143,7 @@ insn_squash () {
else
if test -n "$from"
then
- make_squash_message_multiple "$sha1"
+ make_squash_message_multiple "$sha1" >"$MSG"
else
make_squash_message "$sha1" >"$MSG"
fi
###
Johannes Schindelin wrote:
> > > +peek_next_command () {
> > > + sed -n -e '1s/ .*$//p' <"$TODO"
> > > +}
> >
> > ... which could respond "the next command is '#' (comment)", so we are
> > actively counting a comment as a step here. Does this contradict with the
> > mental note we made earlier, and if so, does the discrepancy hurt us
> > somewhere in this program?
Yes it does hurt ;-)
> Yes, this is wrong. it must be
>
> sed -n -e '/^#/d' -e '1s .*$//p' < "$TODO"
Thanks ;)
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -273,7 +273,7 @@ make_squash_message_multiple () {
}
peek_next_command () {
- sed -n -e '1s/ .*$//p' <"$TODO"
+ sed -n -e '/^#/d' -e '1s/ .*$//p' <"$TODO"
}
# If $1 is a mark, make a ref from it; otherwise keep it.
###
Junio C Hamano wrote:
> > +# If $1 is a mark, make a ref from it; otherwise keep it
> > +mark_to_ref () {
> > + arg="$1"
> > + ref=$(expr "x$arg" : 'x:0*\([0-9][0-9]*\)$')
>
> You might want to leave comments to describe constraints that led to this
> slightly awkward regexp:
>
> * :0 is allowed
> * :01 is the same as :1
Right.
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -270,7 +270,10 @@ peek_next_command () {
sed -n -e '1s/ .*$//p' <"$TODO"
}
-# If $1 is a mark, make a ref from it; otherwise keep it
+# If $1 is a mark, make a ref from it; otherwise keep it.
+# Note on marks:
+# * :0 is allowed
+# * :01 is the same as :1
mark_to_ref () {
arg="$1"
ref=$(expr "x$arg" : 'x:0*\([0-9][0-9]*\)$')
###
Junio C Hamano wrote:
> > +strategy_check () {
> > + case "$1" in
> > + resolve|recursive|octopus|ours|subtree|theirs)
> > + return
> > + ;;
> > + esac
> > + todo_warn "Strategy '$1' not known."
> > +}
>
> Hmm. Do we need to maintain list of available strategies here and then in
> git-merge separately?
No, this is potentially-error-prone at large.
But I want a check for this, so I'd vote for a
--list-strategies
feature in the builtin-merge.
(Before writing the quoted code, I was looking for such a feature, but
haven't found. But since builtin-merge is in work I didn't want send
patches on git-merge.sh)
Johannes Schindelin wrote:
> I'd not check in sequencer for the strategy. Especially given that we
> want to support user-written strategies in the future.
I don't know how this is planned to look like, but perhaps --list-strategies
may make sense here, too.
Also, the shell completion scripts could use that.
Junio C Hamano wrote:
> > +### Author script functions
> > +
> > +clean_author_script () {
> > + cat "$ORIG_AUTHOR_SCRIPT" >"$AUTHOR_SCRIPT"
> > +}
> > +
> > +# Take "Name <e-mail>" in stdin and outputs author script
> > +make_author_script_from_string () {
> > + sed -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME="\1"\
> > +GIT_AUTHOR_EMAIL="\2"\
> > +GIT_AUTHOR_DATE=/'
> > +}
>
> If you are going to "."-source or eval the output from this, you would
> need to quote the values a lot more robustly, wouldn't you? Is this safe
> against shell metacharacters in names, mails and/or space between unixtime
> and the timezone information?
Time is set empty here.
It is not save according to shell metacharacters in name or e-mail and I
knew that when writing.
Of course, a
First "nick" Sure
author will get problems here. ;-)
And as long as nobody is named $(rm -rf "$HOME") [1], I thought this is
sufficient for the prototype.
But...
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -520,7 +520,7 @@ clean_author_script () {
# Take "Name <e-mail>" in stdin and outputs author script
make_author_script_from_string () {
- sed -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME="\1"\
+ sed -e 's/[$"\\]/\\&/g' -e 's/^\(.*\) <\(.*\)>.*$/GIT_AUTHOR_NAME="\1"\
GIT_AUTHOR_EMAIL="\2"\
GIT_AUTHOR_DATE=/'
}
@@ -779,7 +779,7 @@ insn_patch () {
if test -z "$AUTHOR"
then
- sed -n -e '
+ sed -e 's/[$"\\]/\\&/g' -n -e '
s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
###
Is escaping $, " and \ enough?
> > + if test -z "$AUTHOR"
> > + then
> > + sed -n -e '
> > + s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
> > + s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
> > + s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
> > + ' <"$infofile" >>"$AUTHOR_SCRIPT"
>
> The same comment on quoting applies here, I think.
Right.
> > + if test -n "$failed"
> > + then
> > + # XXX: This is just a stupid hack:
> > + with_author git apply $apply_opts --reject --index "$PATCH"
>
> Please don't do this without being asked, if you are planning to use this
> in "am" when 3-way fallback was not asked. It _may_ make sense to give an
> option to the users to ask for .rej if they prefer to work that way better
> than working with 3-way merge fallback, but doing this without being asked
> is not acceptable.
The --reject was just a mind marker for that what I actually think is
useful and less annoying than the current behavior:
> > + die_to_continue 'Patch failed. See the .rej files.'
> > + # XXX: We actually needed a git-apply flag that creates
> > + # conflict markers and sets the DIFF_STATUS_UNMERGED flag.
>
> That is what -3way is all about, and this codepath is when the user did
> not ask for it, isn't it?
Now imagine you apply a patch that cannot be applied 100% cleanly and
you don't have the 3-way base in the repo. You know what happens?
Yes, the patch is completly rejected, because apply is atomic.
And I think a git-apply option that results in a non-atomic behavior,
that creates conflict markers (and no .rej files), would be a great
usability feature for the "patch" insn in sequencer.
I'd even suggest to make that the default, but that's debatable.
Btw, I removed the mind marker:
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -836,9 +836,7 @@ insn_patch () {
if test -n "$failed"
then
- # XXX: This is just a stupid hack:
- with_author git apply $apply_opts --reject --index "$PATCH"
- die_to_continue 'Patch failed. See the .rej files.'
+ die_to_continue 'Patch failed.'
# XXX: We actually needed a git-apply flag that creates
# conflict markers and sets the DIFF_STATUS_UNMERGED flag.
fi
###
> > +# Check the "pick" instruction
> > +check_pick () {
> > + revert=
> > + mainline=
> > + while test $# -gt 1
> > + do
> > ...
> > + done
> > +
> > + if test -n "$mainline"
> > + then
> > + test -z "$revert" ||
> > + todo_error "Cannot use $revert together with --mainline."
>
> Why not? If you have this...
>
> ---A---C---D
> /
> ---B
>
> and you are at D, you may want to undo the merge you made at C and go back
> to either A or B, which essentially is same as cherry-picking diff between
> C and D on top of either A or B. Both are valid operations aren't they?
Jakub already asked. I told him, that git-revert doesn't know --mainline,
but it seems that this was wrong. So:
--- a/git-sequencer.sh
+++ b/git-sequencer.sh
@@ -879,13 +879,11 @@ $OPTIONS_GENERAL
# Check the "pick" instruction
check_pick () {
- revert=
mainline=
while test $# -gt 1
do
case "$1" in
-R)
- revert="$1"
;;
--mainline)
shift
@@ -913,9 +911,6 @@ check_pick () {
if test -n "$mainline"
then
- test -z "$revert" ||
- todo_error "Cannot use $revert together with --mainline."
-
parents=$(count_parents "$1")
test "$parents" -lt "$mainline" &&
todo_error "Commit has only $parents (less than $mainline) parents."
###
Regards and big thanks for the fast reply,
Stephan
--
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
^ 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