* Re: Undoing merges
From: Matthieu Moy @ 2009-12-08 17:48 UTC (permalink / raw)
To: Richard; +Cc: git
In-Reply-To: <8440EA2C12E50645A68C4AA98871665131D8D8@SERVER.webdezign.local>
"Richard" <richard@webdezign.co.uk> writes:
> Hi git list,
>
> I'm trying to find out how to undo a merge.
When sitting on a merge commit,
git reset --merge HEAD^
will undo this merge commit (i.e. pretend the merge has never occured,
at least in your branch). Don't do that if you already published this
merge commit.
> I know that my branches are independent and that I can just carry on
> working on them and merge again later, but I'm just trying to keep
> my revision graph tidier. Should I even be undoing merges?
If it's about cleaning up your history, "git rebase" is your friend,
too (with the same limitation: don't do that on published history). By
default, it does some kind of history flattening.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: help: bisect single file from repos
From: Junio C Hamano @ 2009-12-08 18:35 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Michael J Gruber, wharms
In-Reply-To: <4B1E5796.2090201@bfs.de>
walter harms <wharms@bfs.de> writes:
> Christian Couder schrieb:
> ...
> i am sorry, i am not familiar with git and when i am stating i am looking
> for examples first. the examples in my man page are like
> git bisect start v2.6.20-rc6 v2.6.20-rc4
> there is nothing like:
> git bisect start 6a87a68a6a8 65a76a8a68a7
>
> I ASSUME that you can use tags like "v2.6.20-rc6" and commit-id like "6a87a68a6a8"
> interchangeable but that was not clear from beginning.
The misconception here is "naming commit is done completely differently
from naming branches and tags", which is false in the git land, but it may
hold true in some other systems (e.g. Subversion, which made them into
different dimensions by making branches and tags into a location in a
larger whole tree namespace when their commits are named by serial version
number of that whole tree namespace).
We can call this a misconception, tell users to learn how things work
before using git (sometimes unlearning CVS and Subversion helps for this
exact reason), dismiss the issue and move on. But I wonder if there is
something we _could_ have done better in the documentation area to avoid
this from the beginning, iow, make it easier to "learn how things work
before using"? I think there is a lesson to be learned by us in here, and
I'd like to hear comments and improvement suggestions, especially from
"usability" and "friendly to new people" advocates.
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-08 19:11 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: git, Junio C Hamano
In-Reply-To: <20091208164449.GA32204@redhat.com>
On 2009.12.08 18:44:49 +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 08, 2009 at 05:37:37PM +0100, Björn Steinbrink wrote:
> > On 2009.12.08 18:14:07 +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 08, 2009 at 05:08:22PM +0100, Björn Steinbrink wrote:
> > > > On 2009.12.08 16:47:42 +0200, Michael S. Tsirkin wrote:
> > > > > Add --revisions flag to rebase, so that it can be used
> > > > > to apply an arbitrary range of commits on top
> > > > > of a current branch.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >
> > > > > I've been wishing for this functionality for a while now,
> > > > > so here goes. This isn't yet properly documented and I didn't
> > > > > write a test, but the patch seems to work fine for me.
> > > > > Any early flames/feedback?
> > > >
> > > > This pretty much reverses what rebase normally does. Instead of "rebase
> > > > this onto that" it's "'rebase' that onto this". And instead of updating
> > > > the branch head that got rebased, the, uhm, "upstream" gets updated.
> > >
> > > The last sentence is wrong I think - it is still the branch head that
> > > is updated.
> >
> > But you don't rebase the branch head. Before the rebase, the branch head
> > doesn't reference the commits that get rebased. For example:
> >
> > git checkout bar
> > git rebase --revisions foo bar
> >
> > You "rebase" the commits in foo's history, but you update bar.
>
> Yes, that's the who point of the patch.
Yes, and it's "backwards" compared to the existing "rebase" modes, but
more like "cherry-pick".
> The above applies a single commit, foo, on top of current branch bar.
Hm, no. I expected it to turn all commits reachable from foo into
patches and applying them to bar. But actually, that should hit the
special <since> mode of format-patch. So
git rebase --revisions foo bar
is (with your patch) actually the same as
git rebase foo bar
So actually the example should have been:
git rebase --root --revisions foo bar
Both invocations probably mess up the diff-stat as that becomes:
git diff --stat --summary foo
So it creates a diffstat of the diff from the working tree to "foo",
which can't be right.
>
> > WRT the result, the above command should be equivalent to:
> > git checkout bar
> > git reset --hard foo
> > git rebase --root --onto ORIG_HEAD;
> >
> > And here, the commits currently reachable through "bar" are rebased, and
> > "bar" also gets updated.
>
> So this
> 1. won't be very useful, as you show it is easy
> to achieve with existing commands.
One can "almost" achieve it.
git rebase --revision A..B foo
is about the same as:
git checkout foo
git reset --hard B
git rebase --onto ORIG_HEAD A
But:
a) The "reset --hard" obviously lacks the safety checks for clean
index/working tree.
b) "git rebase --abort" won't take you back to your initial state, but
to B.
c) It's not really obvious that you can do it and how to do it.
Another possibility would be:
git checkout B^0 # detach HEAD at B
git rebase foo # rebase onto foo
git checkout foo
git merge HEAD@{1} # Fast-forwards foo to the rebased stuff
That fixes a), avoid b) [because you don't mess up any branch head
early] but is still subject to c).
And for both methods, the ORIG_HEAD and HEAD@{1} arguments are somewhat
"unstable", e.g. checking out the wrong branch head first, and only then
the correct one, you'd have to use HEAD@{2} instead of HEAD@{1} (because
the reflog for HEAD got a new entry).
So you can already do what you want to do, but wrapping it in a single
porcelain might still be useful because it's obviously a lot easier and
safer that way. That said, I wonder what kind of workflow you're using
though, and why you require that feature. I've never needed something
like that.
> 2. interprets "foo" as branch name as opposed to
> revision range.
Well, a single committish is a "range" as far as the range-based
commands are concerned, e.g. "git log master" treats "master" to mean
all commits reachable it. If "rebase --revisions master" would do the
same, that's at least consistent (and for single commit picks, there's
already cherry-pick). The problem with your patch is that it passes the
revision argument to format-patch as is, and:
git format-patch foo
is the same as
git format-patch foo..HEAD
> OTOH, rebase --revisions as I implemented is a "smarter cherry-pick"
> which can't easily be achieved with existing commands, especially if
> you add "-i".
And that "is a 'smarter cherry-pick'" is why I think that rebase is
actually the wrong command to get that feature. While rebase internally
does just mass-cherry-picking, it does that with commits in the current
branch onto a specified branch. The --revisions flag makes it do things
the other way around.
Björn
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Björn Steinbrink @ 2009-12-08 19:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: git, Junio C Hamano
In-Reply-To: <20091208164904.GB32204@redhat.com>
On 2009.12.08 18:49:04 +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 08, 2009 at 05:41:13PM +0100, Björn Steinbrink wrote:
> > > > Also, AFAICT this needs to be called like this:
> > > > git rebase --revisions foo..bar HEAD
> > > >
> > > > Changing the meaning of the <upstream> argument and relying on the fact
> > > > that <newbase> defaults to <upstream>. If such a thing gets added, it
> > > > should rather work like --root, not using <upstream> at all, but --onto
> > > > <newbase> only. Maybe defaulting to HEAD for <newbase> and making --onto
> > > > optional, as it's reversed WRT what it does compared to the usual
> > > > rebase.
> > >
> > > Sorry, I had trouble parsing the above. Could you suggest e.g. how the
> > > help line should look?
> >
> > Current:
> > git rebase [-i | --interactive] [options] [--onto <newbase>]
> > <upstream> [<branch>]
> > git rebase [-i | --interactive] [options] --onto <newbase>
> > --root [<branch>]
> >
> > Add:
> > git rebase [-i | --interactive] [options] --revisions <range> [<branch>]
> >
> > (Thinking about it, I guess an explicit --onto makes no sense with the
> > --revisions flag)
>
> I agree.
> So this is different from what I implemented basically only in that
> we should disallow combining --onto with --revisions. Right?
It also drops <upstream>, because that makes no sense with --revisions.
So the only mandatory argument is the revision range.
Björn
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning
From: Ramsay Jones @ 2009-12-08 19:48 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Marius Storm-Olsen, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <200912051257.21386.j6t@kdbg.org>
Johannes Sixt wrote:
> On Samstag, 5. Dezember 2009, Ramsay Jones wrote:
>> The patch is still marked RFC because:
>> - I'm still not sure if the flexibility to support both 32- and 64-bit
>> time_t is required.
>> - should -D_USE_32BIT_TIME_T be added to the Makefile?
>
> If *not* using -D_USE_32BIT_TIME_T produces a build or code base that is in
> some way superior, why should we require it? For example, its absence could
> help a 64bit build.
Indeed.
I only added the second bullet because you seemed to be advocating it, in your
last email, so I'm waiting for your answer on that. :-D
Unfortunately the patch can not be a simple as the first version (Dscho was
quite right to complain about adding too much clutter to mingw.h with msvc
related code), but the moral equivalent would have msvc.h look like:
25 /* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead
26 * of fstat(). We add the declaration of these functions here, suppressing
27 * the corresponding declarations in mingw.h, so that we can use the
28 * appropriate structure type (and function) names from the msvc headers.
29 */
30 #define stat _stat64
31 int mingw_lstat(const char *file_name, struct stat *buf);
32 int mingw_fstat(int fd, struct stat *buf);
33 #define fstat mingw_fstat
34 #define lstat mingw_lstat
35 #define _stat64(x,y) mingw_lstat(x,y)
36 #define ALREADY_DECLARED_STAT_FUNCS
37
38 #include "compat/mingw.h"
39
40 #undef ALREADY_DECLARED_STAT_FUNCS
This works fine, *provided* you do not need to compile with -D_USE_32BIT_TIME_T,
which would produce this warning:
...mingw.c(223) : warning C4133: 'function' : incompatible types - \
from '_stat64 *' to '_stat32i64 *'
This would actually be *worse* than the original code, since the struct _stat64
would not have the same "shape" as the struct _stat32i64; it would not write
outside of the allocated memory, at least, but the time fields would obviously
be written to the wrong offsets etc,. In the original code, the struct _stati64
would have the correct "shape", since the time fields are declared with time_t.
At first I thought there would be no need to set _USE_32BIT_TIME_T. After some
thought, however, I could not be confident that it would *never* be necessary.
(my only concern was to revert to 32-bit time_t, perhaps only temporarily, while
fixing a breakage; I did not consider wanting to keep "compatibility" with the
MinGW code). This lead to the more complicated/flexible RFC patch.
I was hoping for someone to say: "Hey, we will *never* need to compile with
-D_USE_32BIT_TIME_T, so just get rid of those #if conditionals and simplify
the code ...". :-P
Since nobody has said any such thing, I have to conclude that the extra
flexibility is required. That being the case, I have to be happy with the
patch as-is and propose to remove the RFC.
Before I do that, do you have any further comments or concerns about the
v2 patch that you want me to address?
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option
From: Jay Soffian @ 2009-12-08 20:01 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, David Aguilar, Junio C Hamano
Add SourceGear DiffMerge to the set of built-in diff/merge tools, and update
bash completion and documentation.
---
Documentation/git-difftool.txt | 2 +-
Documentation/git-mergetool.txt | 2 +-
Documentation/merge-config.txt | 4 ++--
contrib/completion/git-completion.bash | 2 +-
git-mergetool--lib.sh | 22 ++++++++++++++++++++--
5 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 8e9aed6..28178da 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -31,7 +31,7 @@ OPTIONS
Use the diff tool specified by <tool>.
Valid merge tools are:
kdiff3, kompare, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff,
- ecmerge, diffuse, opendiff, p4merge and araxis.
+ ecmerge, diffuse, opendiff, p4merge, araxis and diffmerge.
+
If a diff tool is not specified, 'git-difftool'
will use the configuration variable `diff.tool`. If the
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 4a6f7f3..7f00269 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -27,7 +27,7 @@ OPTIONS
Use the merge resolution program specified by <tool>.
Valid merge tools are:
kdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, ecmerge,
- diffuse, tortoisemerge, opendiff, p4merge and araxis.
+ diffuse, tortoisemerge, opendiff, p4merge, araxis and diffmerge.
+
If a merge resolution program is not specified, 'git-mergetool'
will use the configuration variable `merge.tool`. If the
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index a403155..a68a205 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -23,8 +23,8 @@ merge.tool::
Controls which merge resolution program is used by
linkgit:git-mergetool[1]. Valid built-in values are: "kdiff3",
"tkdiff", "meld", "xxdiff", "emerge", "vimdiff", "gvimdiff",
- "diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis" and
- "opendiff". Any other value is treated is custom merge tool
+ "diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis", "opendiff"
+ and "diffmerge". Any other value is treated is custom merge tool
and there must be a corresponding mergetool.<tool>.cmd option.
merge.verbosity::
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7c18b0c..5cc5ee7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -975,7 +975,7 @@ _git_diff ()
}
__git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
- tkdiff vimdiff gvimdiff xxdiff araxis p4merge
+ tkdiff vimdiff gvimdiff xxdiff araxis p4merge diffmerge
"
_git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 5b62785..5b29fef 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,7 +46,8 @@ check_unchanged () {
valid_tool () {
case "$1" in
kdiff3 | tkdiff | xxdiff | meld | opendiff | \
- emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
+ emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge | \
+ diffmerge)
;; # happy
tortoisemerge)
if ! merge_mode; then
@@ -297,6 +298,23 @@ run_merge_tool () {
>/dev/null 2>&1
fi
;;
+ diffmerge)
+ if merge_mode; then
+ if $base_present; then
+ "$merge_tool_path" -nosplash -merge -result="$MERGED" \
+ "$LOCAL" "$BASE" "$REMOTE"
+ >/dev/null 2>&1
+ else
+ "$merge_tool_path" -nosplash -merge \
+ "$LOCAL" "$MERGED" "$REMOTE"
+ >/dev/null 2>&1
+ fi
+ status=$?
+ else
+ "$merge_tool_path" -nosplash "$LOCAL" "$REMOTE" \
+ >/dev/null 2>&1
+ fi
+ ;;
*)
merge_tool_cmd="$(get_merge_tool_cmd "$1")"
if test -z "$merge_tool_cmd"; then
@@ -336,7 +354,7 @@ guess_merge_tool () {
else
tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
fi
- tools="$tools gvimdiff diffuse ecmerge p4merge araxis"
+ tools="$tools gvimdiff diffuse ecmerge p4merge araxis diffmerge"
fi
case "${VISUAL:-$EDITOR}" in
*vim*)
--
1.6.6.rc1.296.ge77fc.dirty
^ permalink raw reply related
* Re: [PATCH RFC] rebase: add --revisions flag
From: Michael S. Tsirkin @ 2009-12-08 20:00 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: git, Junio C Hamano
In-Reply-To: <20091208191107.GA4103@atjola.homenet>
On Tue, Dec 08, 2009 at 08:11:07PM +0100, Björn Steinbrink wrote:
> On 2009.12.08 18:44:49 +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 08, 2009 at 05:37:37PM +0100, Björn Steinbrink wrote:
> > > On 2009.12.08 18:14:07 +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 08, 2009 at 05:08:22PM +0100, Björn Steinbrink wrote:
> > > > > On 2009.12.08 16:47:42 +0200, Michael S. Tsirkin wrote:
> > > > > > Add --revisions flag to rebase, so that it can be used
> > > > > > to apply an arbitrary range of commits on top
> > > > > > of a current branch.
> > > > > >
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > I've been wishing for this functionality for a while now,
> > > > > > so here goes. This isn't yet properly documented and I didn't
> > > > > > write a test, but the patch seems to work fine for me.
> > > > > > Any early flames/feedback?
> > > > >
> > > > > This pretty much reverses what rebase normally does. Instead of "rebase
> > > > > this onto that" it's "'rebase' that onto this". And instead of updating
> > > > > the branch head that got rebased, the, uhm, "upstream" gets updated.
> > > >
> > > > The last sentence is wrong I think - it is still the branch head that
> > > > is updated.
> > >
> > > But you don't rebase the branch head. Before the rebase, the branch head
> > > doesn't reference the commits that get rebased. For example:
> > >
> > > git checkout bar
> > > git rebase --revisions foo bar
> > >
> > > You "rebase" the commits in foo's history, but you update bar.
> >
> > Yes, that's the who point of the patch.
>
> Yes, and it's "backwards" compared to the existing "rebase" modes, but
> more like "cherry-pick".
>
> > The above applies a single commit, foo, on top of current branch bar.
>
> Hm, no. I expected it to turn all commits reachable from foo into
> patches and applying them to bar. But actually, that should hit the
> special <since> mode of format-patch. So
> git rebase --revisions foo bar
> is (with your patch) actually the same as
> git rebase foo bar
>
> So actually the example should have been:
> git rebase --root --revisions foo bar
>
> Both invocations probably mess up the diff-stat as that becomes:
> git diff --stat --summary foo
> So it creates a diffstat of the diff from the working tree to "foo",
> which can't be right.
>
> >
> > > WRT the result, the above command should be equivalent to:
> > > git checkout bar
> > > git reset --hard foo
> > > git rebase --root --onto ORIG_HEAD;
> > >
> > > And here, the commits currently reachable through "bar" are rebased, and
> > > "bar" also gets updated.
> >
> > So this
> > 1. won't be very useful, as you show it is easy
> > to achieve with existing commands.
>
> One can "almost" achieve it.
> git rebase --revision A..B foo
>
> is about the same as:
> git checkout foo
> git reset --hard B
> git rebase --onto ORIG_HEAD A
>
> But:
> a) The "reset --hard" obviously lacks the safety checks for clean
> index/working tree.
> b) "git rebase --abort" won't take you back to your initial state, but
> to B.
> c) It's not really obvious that you can do it and how to do it.
>
> Another possibility would be:
>
> git checkout B^0 # detach HEAD at B
> git rebase foo # rebase onto foo
> git checkout foo
> git merge HEAD@{1} # Fast-forwards foo to the rebased stuff
>
> That fixes a), avoid b) [because you don't mess up any branch head
> early] but is still subject to c).
>
> And for both methods, the ORIG_HEAD and HEAD@{1} arguments are somewhat
> "unstable", e.g. checking out the wrong branch head first, and only then
> the correct one, you'd have to use HEAD@{2} instead of HEAD@{1} (because
> the reflog for HEAD got a new entry).
>
> So you can already do what you want to do, but wrapping it in a single
> porcelain might still be useful because it's obviously a lot easier and
> safer that way. That said, I wonder what kind of workflow you're using
> though, and why you require that feature. I've never needed something
> like that.
I need this often for many reasons:
- Imagine developing a patchset with a complex bugfix on master branch.
Then I decide to also apply (backport) this patchset to stable branch.
- Imagine developing a bugfix/feature patchset on master branch.
Then I decide the patchset is too large/unsafe and want to
switch it to staging branch.
- I have a large queue of patches on staging branch, I decide that
a range of patches is mature enough for master.
And I often need -i to inspec/edit patches while doing this,
even though I can rebase -i later, but that would mean
figuring which commit to pass to rebase -i.
> > 2. interprets "foo" as branch name as opposed to
> > revision range.
>
> Well, a single committish is a "range" as far as the range-based
> commands are concerned, e.g. "git log master" treats "master" to mean
> all commits reachable it. If "rebase --revisions master" would do the
> same, that's at least consistent (and for single commit picks, there's
> already cherry-pick). The problem with your patch is that it passes the
> revision argument to format-patch as is, and:
> git format-patch foo
> is the same as
> git format-patch foo..HEAD
>
>
> > OTOH, rebase --revisions as I implemented is a "smarter cherry-pick"
> > which can't easily be achieved with existing commands, especially if
> > you add "-i".
>
> And that "is a 'smarter cherry-pick'" is why I think that rebase is
> actually the wrong command to get that feature. While rebase internally
> does just mass-cherry-picking, it does that with commits in the current
> branch onto a specified branch. The --revisions flag makes it do things
> the other way around.
>
> Björn
Well, implemenation-wise, teaching cherry-pick about multiple
commits seems very hard to me. We would need to teach it about
all the flags that rebase has to patch queue management.
So I can't implement it. Can you?
--
MST
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Junio C Hamano @ 2009-12-08 20:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: git
In-Reply-To: <20091208144740.GA30830@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> Add --revisions flag to rebase, so that it can be used
> to apply an arbitrary range of commits on top
> of a current branch.
Many people wanted to have "pick many commits onto the current HEAD" and I
think it would be a natural, uncontroversial and welcome addition to allow
"git cherry-pick A..B". In fact, historically, people who wanted to have
"pick many commits" complained that the "rebase" interface was backwards,
because it works in the _wrong_ direction for _their_ usecase. Of course,
when you _are_ rebasing a branch on top of some other branch, the way
"rebase" currently works is the _right_ direction.
But I think it is a reasonable thing to _implement_ the feature to
range-pick commits reusing the sequencing logic already in "rebase" and
"rebase -i". That essentially is what we wanted to do with "git
sequencer" that would be a sequencing logic backend shared among rebase,
cherry-pick, and perhaps am.
So perhaps a good way to move forward is to teach "git cherry-pick A..B"
to be a thin wrapper that invokes a new hidden mode of operation added to
"rebase" that is not advertised to the end user.
I would suggest calling the option to invoke that hidden mode not
"--revisions", but "--reverse" or "--opposite" or something of that
nature, though. It makes "rebase" work in different direction.
^ permalink raw reply
* Re: [PATCH RFC] rebase: add --revisions flag
From: Sverre Rabbelier @ 2009-12-08 20:29 UTC (permalink / raw)
To: Junio C Hamano, Stephan Beyer, Christian Couder, Daniel Barkalow; +Cc: Git List
In-Reply-To: <7vfx7lcj18.fsf@alter.siamese.dyndns.org>
Heya,
On Tue, Dec 8, 2009 at 21:22, Junio C Hamano <gitster@pobox.com> wrote:
> But I think it is a reasonable thing to _implement_ the feature to
> range-pick commits reusing the sequencing logic already in "rebase" and
> "rebase -i". That essentially is what we wanted to do with "git
> sequencer" that would be a sequencing logic backend shared among rebase,
> cherry-pick, and perhaps am.
Speaking of which, what's the status of git sequencer? I seem to
remember some activity recently to slowly rewrite git rebase in c, but
I haven't seen anything since then. Is it still moving forward? Is
anyone interested in doing so? Just curious...
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] MSVC: Fix an "incompatible pointer types" compiler warning
From: Johannes Sixt @ 2009-12-08 20:31 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Marius Storm-Olsen, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <4B1EAD9A.3090205@ramsay1.demon.co.uk>
On Dienstag, 8. Dezember 2009, Ramsay Jones wrote:
> 30 #define stat _stat64
> 31 int mingw_lstat(const char *file_name, struct stat *buf);
> 32 int mingw_fstat(int fd, struct stat *buf);
> 33 #define fstat mingw_fstat
> 34 #define lstat mingw_lstat
> 35 #define _stat64(x,y) mingw_lstat(x,y)
> 36 #define ALREADY_DECLARED_STAT_FUNCS
> 37
> 38 #include "compat/mingw.h"
> 39
> 40 #undef ALREADY_DECLARED_STAT_FUNCS
>
> This works fine, *provided* you do not need to compile with
> -D_USE_32BIT_TIME_T, which would produce this warning:
>
> ...mingw.c(223) : warning C4133: 'function' : incompatible types - \
> from '_stat64 *' to '_stat32i64 *'
>
> This would actually be *worse* than the original code, since the struct
> _stat64 would not have the same "shape" as the struct _stat32i64; ...
To cut this short: According to your explanations, using -D_USE_32BIT_TIME_T
with MSVC is bad. Please reroll without references to _USE_32BIT_TIME_T.
-- Hannes
^ permalink raw reply
* delete remote branches
From: Patrick Grimard @ 2009-12-08 20:35 UTC (permalink / raw)
To: git
I'm experimenting with Git, just trying to get a feel for it. I
created a bare git repo on a server and pushed a local git repo (let's
call it DIR1) from my PC to the server. Then from another directory
on my local PC (let's call it DIR2), I cloned the repo on the server
and checked out a new branch named testBranch. Did some playing with
branches to see how they worked, like pushing the branch to the server
from DIR2 and fetching it from DIR1, merging it into master and then
pushing master back to the server. Now from DIR2, I fetched master
from the server, merged in the changes and deleted the remote branch
called testBranch using the command "git push origin :testBranch"
which seemed to work fine since the command "git branch -r" no longer
lists the remote branch. However from DIR1 if I do "git branch -r" I
still see the remote branch and can't seem to delete it using the
above method. Any idea why this is happening?
Thanks in advance.
Patrick
^ permalink raw reply
* [PATCH 2/2] git-svn: Move setting of svn.authorsfile in clone to between init and fetch
From: Alex Vandiver @ 2009-12-08 20:54 UTC (permalink / raw)
To: git; +Cc: Eric Wong
In-Reply-To: <1260305651-11111-1-git-send-email-alex@chmrr.net>
If a clone errors out because of a missing author, or user interrupt,
this allows `git svn fetch` to resume seamlessly, rather than forcing
the user to re-provide the path to the authors file.
Signed-off-by: Alex Vandiver <alex@chmrr.net>
---
git-svn.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index cf5e75e..3c3e0e0 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -394,9 +394,9 @@ sub cmd_clone {
$path = basename($url) if !defined $path || !length $path;
my $authors_absolute = $_authors ? File::Spec->rel2abs($_authors) : "";
cmd_init($url, $path);
- Git::SVN::fetch_all($Git::SVN::default_repo_id);
command_oneline('config', 'svn.authorsfile', $authors_absolute)
if $_authors;
+ Git::SVN::fetch_all($Git::SVN::default_repo_id);
}
sub cmd_init {
--
1.6.6.rc0.360.gc408
^ permalink raw reply related
* [PATCH 1/2] git-svn: Set svn.authorsfile to an absolute path when cloning
From: Alex Vandiver @ 2009-12-08 20:54 UTC (permalink / raw)
To: git; +Cc: Eric Wong
If --authors-file is passed a relative path, cloning will work, but
future `git svn fetch`es will fail to locate the authors file
correctly. Thus, use File::Spec->rel2abs to determine an absolute
path for the authors file before setting it.
Signed-off-by: Alex Vandiver <alex@chmrr.net>
---
git-svn.perl | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 957d44e..cf5e75e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -392,9 +392,11 @@ sub cmd_clone {
$path = $url;
}
$path = basename($url) if !defined $path || !length $path;
+ my $authors_absolute = $_authors ? File::Spec->rel2abs($_authors) : "";
cmd_init($url, $path);
Git::SVN::fetch_all($Git::SVN::default_repo_id);
- command_oneline('config', 'svn.authorsfile', $_authors) if $_authors;
+ command_oneline('config', 'svn.authorsfile', $authors_absolute)
+ if $_authors;
}
sub cmd_init {
--
1.6.6.rc0.360.gc408
^ permalink raw reply related
* Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug
From: Stephen Boyd @ 2009-12-08 21:56 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200912081729.11890.jnareb@gmail.com>
On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote:
>
> Does the following fixes the issue for IE8 for you (it works for me)?
>
Yes, there is no longer an error but IE8 still locks up and takes about
30 seconds. It doesn't seem to be any more responsive. Isn't putting the
error in a try-catch just papering over the issue?
^ permalink raw reply
* Re: delete remote branches
From: Andreas Schwab @ 2009-12-08 22:06 UTC (permalink / raw)
To: Patrick Grimard; +Cc: git
In-Reply-To: <9cdb17250912081235m2b061ca5x70fe42749b49670f@mail.gmail.com>
Patrick Grimard <pgrimard@gmail.com> writes:
> However from DIR1 if I do "git branch -r" I still see the remote
> branch and can't seem to delete it using the above method.
Try "git remote prune origin".
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Weird message when pulling git version 1.6.6.rc1.39.g9a42
From: Alejandro Riveira @ 2009-12-08 22:05 UTC (permalink / raw)
To: git
$ git pull
remote: Counting objects: 9, done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 5 (delta 4), reused 0 (delta 0)
Unpacking objects: 100% (5/5), done.
From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
6035ccd..a252e74 master -> origin/master
Updating 6035ccd..a252e74
Fast-forward (no commit created; -m option ignored) # what is this ??
net/sctp/sysctl.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
^ permalink raw reply
* Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug
From: Jakub Narebski @ 2009-12-08 22:24 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1260309382.5658.41.camel@swboyd-laptop>
On Tue, 8 Dec 2009, Stephen Boyd wrote:
> On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote:
> >
> > Does the following fixes the issue for IE8 for you (it works for me)?
> >
>
> Yes, there is no longer an error but IE8 still locks up and takes about
> 30 seconds. It doesn't seem to be any more responsive. Isn't putting the
> error in a try-catch just papering over the issue?
Well, I wrote it is *workaround* for IE8 bug, didn't I?
There are actually two separate issues. First is 'blame_incremental'
freezing browser (making it non responsive), second is proper solution
to this bug.
The problem with 'blame_incremental' freezing is that JavaScript is
single-threaded, and that modifying DOM is not lightweight. gitweb.js
should then use technique described in
http://www.nczonline.net/blog/2009/08/11/timed-array-processing-in-javascript/
to avoid freezing browser, and perhaps also some technique to avoid
reflowing (if possible).
The proper solution for IE8 bug would be to use 'progress', 'error'
and 'load' events of XHR 2.0 (XMLHttpRequest specification level 2)
if they are available, instead of mucking with underspecified
'readystatechange' event from XHR 1.0 and timer. But it is a more
complicated solution.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: PATCH/RFC] gitweb.js: Workaround for IE8 bug
From: Jakub Narebski @ 2009-12-08 22:32 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1260309382.5658.41.camel@swboyd-laptop>
Dnia wtorek 8. grudnia 2009 22:56, Stephen Boyd napisał:
> On Tue, 2009-12-08 at 17:29 +0100, Jakub Narebski wrote:
> >
> > Does the following fixes the issue for IE8 for you (it works for me)?
> >
>
> Yes, there is no longer an error but IE8 still locks up and takes about
> 30 seconds. It doesn't seem to be any more responsive. Isn't putting the
> error in a try-catch just papering over the issue?
Does increasing interval in setInterval call at the end of startBlame
finction in gitweb.js from 1000 (1 second) to e.g. 2000 (2 seconds)
help there?
--
Jakub Narebski
Poland
^ permalink raw reply
* false directory/file merge conflict
From: Jeffrey Middleton @ 2009-12-08 22:55 UTC (permalink / raw)
To: git
When a directory (with contents) has changed into a file on one
branch, and another branch attempts to merge it, there's a false
conflict - the directory is processed before its contents are removed.
This is particularly nasty, because "resolving" the non-existent
conflict results in no changes, so future merges will fail in exactly
the same way. Test script and failed merge output below.
Jeffrey
-- 8< --
rm -rf directory-file
mkdir directory-file
cd directory-file
git init
mkdir foo
echo bar > foo/bar
echo bas > foo/baz
git add foo/bar foo/baz
git commit -m "check in files"
git branch foo_unchanged
git rm -r foo
echo foo > foo
git add foo
git commit -m "foo changes from directory to file"
git checkout foo_unchanged
echo foobar > foobar
git add foobar
git commit -m "make a separate change"
git merge master
-- 8< --
CONFLICT (directory/file): There is a directory with name foo in HEAD.
Adding foo as foo~master
Removing foo/bar
Removing foo/baz
Automatic merge failed; fix conflicts and then commit the result.
^ permalink raw reply
* Re: Weird message when pulling git version 1.6.6.rc1.39.g9a42
From: Junio C Hamano @ 2009-12-08 23:33 UTC (permalink / raw)
To: Alejandro Riveira; +Cc: git, Horst H. von Brand, Nanako Shiraishi
In-Reply-To: <hfmijf$dl1$1@ger.gmane.org>
Alejandro Riveira <ariveira@gmail.com> writes:
> $ git pull
> remote: Counting objects: 9, done.
> remote: Compressing objects: 100% (5/5), done.
> remote: Total 5 (delta 4), reused 0 (delta 0)
> Unpacking objects: 100% (5/5), done.
> From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
> 6035ccd..a252e74 master -> origin/master
> Updating 6035ccd..a252e74
> Fast-forward (no commit created; -m option ignored) # what is this ??
> net/sctp/sysctl.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
Thanks for reporting.
Warning about a message that the end user didn't give the command is
clearly wrong. I guess we would want to revert c0ecb07 (git-pull.sh: Fix
call to git-merge for new command format, 2009-12-01), and b81e00a
(git-merge: a deprecation notice of the ancient command line syntax,
2009-11-30).
Reverting them will still keep 76bf488 (Do not misidentify "git merge foo
HEAD" as an old-style invocation, 2009-12-02) that resulted in the change
we are reverting here, so we are still ahead ;-)
http://thread.gmane.org/gmane.comp.version-control.git/134103/focus=134145
-- >8 --
Subject: [PATCH] Revert recent "git merge <msg> HEAD <commit>..." deprecation
This reverts commit c0ecb07048ce2123589a2f077d296e8cf29a9570 "git-pull.sh:
Fix call to git-merge for new command format" and
commit b81e00a965c62ca72a4b9db425ee173de147808d "git-merge: a deprecation
notice of the ancient command line syntax".
They caused a "git pull" (without any arguments, and without any local
commits---only to update to the other side) to warn that commit log
message is ignored because the merge resulted in a fast-forward.
Another possible solution is to add an extra option to "git merge" so that
"git pull" can tell it that the message given is not coming from the end
user (the canned message is passed just in case the merge resulted in a
non-ff and caused commit), but I think it is easier _not_ to deprecate the
old syntax.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-merge.c | 6 ------
git-pull.sh | 6 +++---
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index 56a1bb6..f1c84d7 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -796,11 +796,6 @@ static int suggest_conflicts(void)
return 1;
}
-static const char deprecation_warning[] =
- "'git merge <msg> HEAD <commit>' is deprecated. Please update\n"
- "your script to use 'git merge -m <msg> <commit>' instead.\n"
- "In future versions of git, this syntax will be removed.";
-
static struct commit *is_old_style_invocation(int argc, const char **argv)
{
struct commit *second_token = NULL;
@@ -814,7 +809,6 @@ static struct commit *is_old_style_invocation(int argc, const char **argv)
die("'%s' is not a commit", argv[1]);
if (hashcmp(second_token->object.sha1, head))
return NULL;
- warning(deprecation_warning);
}
return second_token;
}
diff --git a/git-pull.sh b/git-pull.sh
index 502af1a..bfeb4a0 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -216,7 +216,7 @@ fi
merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
test true = "$rebase" &&
- exec git rebase $diffstat $strategy_args --onto $merge_head \
+ exec git-rebase $diffstat $strategy_args --onto $merge_head \
${oldremoteref:-$merge_head}
-exec git merge $verbosity $diffstat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args \
- -m "$merge_name" $merge_head
+exec git-merge $diffstat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args \
+ "$merge_name" HEAD $merge_head $verbosity
--
1.6.6.rc1.42.gf9ad7
^ permalink raw reply related
* Re: [REROLL PATCH 1/8] Add remote helper debug mode
From: Junio C Hamano @ 2009-12-08 23:34 UTC (permalink / raw)
To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260278177-9029-2-git-send-email-ilari.liusvaara@elisanet.fi>
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> Remote helpers deadlock easily, so support debug mode which shows the
> interaction steps.
Also new helper functions, sendline(), recvline(), write_constant() and
xchgline() introduces abstraction that makes the whole thing a lot nicer
to read ;-).
Nice.
^ permalink raw reply
* Re: [REROLL PATCH 2/8] Support mandatory capabilities
From: Junio C Hamano @ 2009-12-08 23:34 UTC (permalink / raw)
To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260278177-9029-3-git-send-email-ilari.liusvaara@elisanet.fi>
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> diff --git a/transport-helper.c b/transport-helper.c
> index a721dc2..f977d28 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -93,25 +93,39 @@ static struct child_process *get_helper(struct transport *transport)
>
> data->out = xfdopen(helper->out, "r");
> while (1) {
> + const char *capname;
> + int mandatory = 0;
> recvline(data, &buf);
> ...
> + } else if (mandatory) {
> + fflush(stderr);
> + die("Unknown madatory capability %s. This remote "
> + "helper probably needs newer version of Git.\n",
> + capname);
Why fflush() here? Is the reason for needing to flush stderr before
letting die() to write into it very specific to this codepath, or shared
among other callers of die()? I am wondering if we should add this
fflush() to report() in usage.c instead.
^ permalink raw reply
* Re: [REROLL PATCH 3/8] Pass unknown protocols to external protocol handlers
From: Junio C Hamano @ 2009-12-08 23:35 UTC (permalink / raw)
To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260278177-9029-4-git-send-email-ilari.liusvaara@elisanet.fi>
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> +const char *remove_ext_force(const char *url)
> +{
> + const char *url2 = url;
> + const char *first_colon = NULL;
> +
> + if (!url)
> + return NULL;
> +
> + while (*url2 && !first_colon)
> + if (*url2 == ':')
> + first_colon = url2;
> + else
> + url2++;
> +
> + if (first_colon && first_colon[1] == ':')
> + return first_colon + 2;
> + else
> + return url;
> +}
Sorry, I am slow today, but is this any different from:
if (url) {
const char *colon = strchr(url, ':');
if (colon && colon[1] == ':')
return url2 + 2;
}
return url;
> diff --git a/transport.c b/transport.c
> index 3eea836..e42a82b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -780,6 +780,58 @@ static int is_file(const char *url)
> return S_ISREG(buf.st_mode);
> }
>
> +static const char *strchrc(const char *str, int c)
> +{
> + while (*str)
> + if (*str == c)
> + return str;
> + else
> + str++;
> + return NULL;
> +}
I cannot spot how this is different from strchr()...
> +static int is_url(const char *url)
> +{
> + const char *url2, *first_slash;
> +
> + if (!url)
> + return 0;
> + url2 = url;
> + first_slash = strchrc(url, '/');
> +
> + /* Input with no slash at all or slash first can't be URL. */
> + if (!first_slash || first_slash == url)
> + return 0;
> + /* Character before must be : and next must be /. */
> + if (first_slash[-1] != ':' || first_slash[1] != '/')
> + return 0;
> + /* There must be something before the :// */
> + if (first_slash == url + 1)
> + return 0;
> + /*
> + * Check all characters up to first slash. Only alpha, num and
> + * colon (":") are allowed. ":" must be followed by ":" or "/".
> + */
Hmm, so "a::::bc:://" is ok?
Is the reason this does not to check the string up to (first_slash-1) for
a valid syntax for <scheme> (as in "<scheme>://") because this potentially
has "<helper>::" in front of it?
I cannot tell if you want to allow "<helper1>::<helper2>::<scheme>://..."
by reading this code.
> +static int external_specification_len(const char *url)
> +{
> + return strchrc(url, ':') - url;
> +}
Does the caller guarantee url has at least one colon in it?
> struct transport *transport_get(struct remote *remote, const char *url)
> {
> struct transport *ret = xcalloc(1, sizeof(*ret));
> @@ -805,30 +857,23 @@ struct transport *transport_get(struct remote *remote, const char *url)
>
> if (remote && remote->foreign_vcs) {
> transport_helper_init(ret, remote->foreign_vcs);
> + } else if (!prefixcmp(url, "rsync:")) {
> ret->get_refs_list = get_refs_via_rsync;
> ret->fetch = fetch_objs_via_rsync;
> ret->push = rsync_transport_push;
> } else if (is_local(url) && is_file(url)) {
> struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
> ret->data = data;
> ret->get_refs_list = get_refs_from_bundle;
> ret->fetch = fetch_refs_from_bundle;
> ret->disconnect = close_bundle;
> + } else if (!is_url(url)
> + || !prefixcmp(url, "file://")
> + || !prefixcmp(url, "git://")
> + || !prefixcmp(url, "ssh://")
> + || !prefixcmp(url, "git+ssh://")
> + || !prefixcmp(url, "ssh+git://")) {
> + /* These are builtin smart transports. */
Hmm, what is !is_url(url) at the beginning for, if this lists "builtin"
smart transports?
> @@ -845,6 +890,21 @@ struct transport *transport_get(struct remote *remote, const char *url)
> data->receivepack = "git-receive-pack";
> if (remote->receivepack)
> data->receivepack = remote->receivepack;
> + } else if (!prefixcmp(url, "http://")
> + || !prefixcmp(url, "https://")
> + || !prefixcmp(url, "ftp://")) {
> + /* These three are just plain special. */
> + transport_helper_init(ret, "curl");
> +#ifdef NO_CURL
> + error("git was compiled without libcurl support.");
> +#endif
> + } else {
> + /* Unknown protocol in URL. Pass to external handler. */
> + int len = external_specification_len(url);
> + char *handler = xmalloc(len + 1);
> + handler[len] = 0;
> + strncpy(handler, url, len);
> + transport_helper_init(ret, handler);
Hmph, external_specification_len() may get passed a colon-less string
after all, I think.
^ permalink raw reply
* Re: [REROLL PATCH 5/8] Support taking over transports
From: Junio C Hamano @ 2009-12-08 23:37 UTC (permalink / raw)
To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260278177-9029-6-git-send-email-ilari.liusvaara@elisanet.fi>
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> diff --git a/transport-helper.c b/transport-helper.c
> index 0e82553..3b7340c 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -22,6 +22,7 @@ struct helper_data
> /* These go from remote name (as in "list") to private name */
> struct refspec *refspecs;
> int refspec_nr;
> + struct git_transport_options gitoptions;
> };
Will we ever have another 'xxxoptions' field in this structure that is not
so git-ish? 'gitoptions' somehow doesn't feel right, unless you plan to
later add scm specific options like 'hgoptions', 'bzroptions' in this
field you need to distinguish "git" one from them.
I know you needed to name the new field to store the transport option
under a different name from the existing 'option' field, but for that
purpose, 'transport_options' might be a more appropriate name.
> @@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)
> die("Unable to run helper: git %s", helper->argv[0]);
> data->helper = helper;
>
> + /*
> + * Open the output as FILE* so strbuf_getline() can be used.
> + * Do this with duped fd because fclose() will close the fd,
> + * and stuff like taking over will require the fd to remain.
> + *
> + */
> + duped = dup(helper->out);
> + if (duped < 0)
> + die_errno("Can't dup helper output fd");
> + data->out = xfdopen(duped, "r");
> +
Neat hack (the kind I often love), but it makes something deep inside me
cringe. This looks fragile and possibly wrong.
Who guarantees that reading from the (FILE *)data->out by strbuf_getline()
that eventually calls into fgetc() does not cause more data be read in the
buffer assiciated with the (FILE *) than we will consume? The other FILE*
you will make out of helper->out won't be able to read what data->out has
already slurped in to its stdio buffer.
The call sequence, after [6/8] is applied as I understand it is:
- _process_connect()
- get_helper()
- start_command() that gives a pipe to read from the helper in
helper->out;
- the above dup dance makes (FILE *)data->out out of a copied fd;
- read from data->out, potentially reading a lot more than
the loop consumes into the stdio buffer of data->out;
- dup dance again to make (FILE *)input;
- read from input, unbuffered.
But if "capabilities" exchange has read past the capability response from
the helper into helper->out inside get_helper(), wouldn't it make the dup
dance to make an extra "input" to read the rest unbuffered moot, as
"input" won't be even reading from the beginning of "the rest"?
^ permalink raw reply
* Re: [REROLL PATCH 6/8] Support remote helpers implementing smart transports
From: Junio C Hamano @ 2009-12-08 23:38 UTC (permalink / raw)
To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260278177-9029-7-git-send-email-ilari.liusvaara@elisanet.fi>
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
> Documentation/git-remote-helpers.txt | 25 +++++++-
> transport-helper.c | 126 ++++++++++++++++++++++++++++++++--
> 2 files changed, 144 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> index 20a05fe..b957813 100644
> --- a/Documentation/git-remote-helpers.txt
> +++ b/Documentation/git-remote-helpers.txt
> @@ -93,6 +93,20 @@ Supported if the helper has the "push" capability.
> +
> Supported if the helper has the "import" capability.
>
> +'connect' <service>::
> + Connects to given service. Stdin and stdout of helper are
A minor point, but in prose, unless it explains how to use "stdin" and
"stdout" as a variable, keyword, etc. in code, I'd prefer to see these
spelled out, like so:
The standard input and output of helpers are connected to ...
> @@ -34,12 +36,12 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)
> die_errno("Full write to remote helper failed");
> }
>
> -static int recvline(struct helper_data *helper, struct strbuf *buffer)
> +static int _recvline(FILE *helper, struct strbuf *buffer)
> {
Not a good naming for two reasons.
- Somebody new to the code wouldn't be able to tell which of recvline()
and _recvline() take helper_data and FILE easily.
- A function name that starts with an underscore, even if it is file
scoped static, is something we tend to avoid. This applies to
_process_connect() in the earlier patch as well.
recvline_fh() vs revline() might be better as most of the interaction in
this layer are done on helper_data, which makes the name recvline() pair
nicely with sendline that also takes helper_data; and the oddball one that
is _not_ an implementation detail (i.e. you have calls outside recvline()
implementation that call _recvline()) hints that it takes filehandle
instead.
^ 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