* Re: Questions/investigations on git-subtree and tags
From: Jeremy Rosen @ 2013-03-07 11:05 UTC (permalink / raw)
To: Paul Campbell; +Cc: git
In-Reply-To: <CALeLG_=YQDrgMfOoR_GyNRexWxDqQGhFubmaYwM6pqXJuLnO6A@mail.gmail.com>
>
> Hi Jérémy,
>
> Git subtree ignores tags from the remote repo.
>
is that a design decision or a case of "not implemented yet"
> To follow a project in a subdirectory I would use git-subtree add
> selecting a branch, not a tag, from the other repo. Then use
> git-subtree pull to keep yourself updated.
>
well... yes, but releases are marked by tags, not branches so what I really want is a tag.
I still use git so I have the possibility to update and can traceback what happened later
> e.g.
>
> To add:
>
> git subtree add --prefix=$subdir $repo $branch
>
> Then to update:
>
> git subtree pull --prefix=$subdir $repo $branch
>
ok, that probably works with branches (didn't test)
> If you make any changes on the branch and wanted to push them back
> you
> could do that with:
>
> git subtree pull --prefix=$subdir $repo2 $branch2
>
> $repo2 and $branch2 would be different from $repo and $branch if you
> wanted to push to your own fork before submitting a pull request.
>
shouldn't there be a subtree split somewhere ? IIUC pull is only merge from the remote to my local repo,
not the other way round
> --
> Paul [W] Campbell
>
^ permalink raw reply
* [PATCH] git svn: ignore partial svn:mergeinfo
From: Jan Pešta @ 2013-03-07 11:28 UTC (permalink / raw)
To: 'Eric Wong'
Cc: git, 'Junio C Hamano', 'Matthieu Moy',
'Sam Vilain'
Currently this is cosmetic change - the merges are ignored, becuase the methods
(lookup_svn_merge, find_rev_before, find_rev_after) are failing on comparing text with number.
See http://www.open.collab.net/community/subversion/articles/merge-info.html
Extract:
The range r30430:30435 that was added to 1.5.x in this merge has a '*' suffix for 1.5.x\www.
This '*' is the marker for a non-inheritable mergeinfo range.
The '*' means that only the path on which the mergeinfo is explicitly set has had this range merged into it.
Signed-off-by: Jan Pesta <jan.pesta@certicon.cz>
---
perl/Git/SVN.pm | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 0ebc68a..74d49bb 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1493,6 +1493,11 @@ sub lookup_svn_merge {
my @merged_commit_ranges;
# find the tip
for my $range ( @ranges ) {
+ if ($range =~ /[*]$/) {
+ warn "W:Ignoring partial merge in svn:mergeinfo "
+ ."dirprop: $source:$range\n";
+ next;
+ }
my ($bottom, $top) = split "-", $range;
$top ||= $bottom;
my $bottom_commit = $gs->find_rev_after( $bottom, 1, $top );
--
1.8.1.msysgit.1
^ permalink raw reply related
* Re: Questions/investigations on git-subtree and tags
From: Paul Campbell @ 2013-03-07 12:02 UTC (permalink / raw)
To: Jeremy Rosen; +Cc: git
In-Reply-To: <1938485067.193912.1362654351290.JavaMail.root@openwide.fr>
On Thu, Mar 7, 2013 at 11:05 AM, Jeremy Rosen <jeremy.rosen@openwide.fr> wrote:
>>
>> Hi Jérémy,
>>
>> Git subtree ignores tags from the remote repo.
>>
>
> is that a design decision or a case of "not implemented yet"
I'm not sure. If you imported all the tags from all your subtrees
repos, you could easily end up with duplicate tags from different
repos. They could be namespaced, but there is no concept of namespace
in git-subtree. That even assumes that you can tag a subtree (I've not
tried).
>> To follow a project in a subdirectory I would use git-subtree add
>> selecting a branch, not a tag, from the other repo. Then use
>> git-subtree pull to keep yourself updated.
>>
>
>
> well... yes, but releases are marked by tags, not branches so what I really want is a tag.
>
> I still use git so I have the possibility to update and can traceback what happened later
>
>> e.g.
>>
>> To add:
>>
>> git subtree add --prefix=$subdir $repo $branch
>>
>> Then to update:
>>
>> git subtree pull --prefix=$subdir $repo $branch
>>
>
>
> ok, that probably works with branches (didn't test)
>
>> If you make any changes on the branch and wanted to push them back
>> you
>> could do that with:
>>
>> git subtree pull --prefix=$subdir $repo2 $branch2
>>
>> $repo2 and $branch2 would be different from $repo and $branch if you
>> wanted to push to your own fork before submitting a pull request.
>>
>
> shouldn't there be a subtree split somewhere ? IIUC pull is only merge from the remote to my local repo,
> not the other way round
Oops, that should have been git subtree push, which uses git subtree
split internally.
--
Paul [W] Campbell
^ permalink raw reply
* Re: Questions/investigations on git-subtree and tags
From: Jeremy Rosen @ 2013-03-07 12:50 UTC (permalink / raw)
To: Paul Campbell; +Cc: git
In-Reply-To: <CALeLG_k=8S0Q-89CUvNPQT91gkJm1NdDN6YtCkdTQZZi9RUjWA@mail.gmail.com>
> >>
> >> Git subtree ignores tags from the remote repo.
> >>
> >
> > is that a design decision or a case of "not implemented yet"
>
> I'm not sure. If you imported all the tags from all your subtrees
> repos, you could easily end up with duplicate tags from different
> repos. They could be namespaced, but there is no concept of namespace
> in git-subtree. That even assumes that you can tag a subtree (I've
> not
> tried).
>
Ok, I can understand that you don't want to import tags for namespace reason, but in that case shouldn't
git subtree add refuse to create a subtree when the tag isn't a commit
or if it allows it, what would be the gracefull way to handle that ?
i'm quite new to git's internals, so I don't really know if/what the right approch would be.
note that all those problems seems to disapear when squash is not used
^ permalink raw reply
* Re: Questions/investigations on git-subtree and tags
From: Paul Campbell @ 2013-03-07 15:00 UTC (permalink / raw)
To: Jeremy Rosen; +Cc: git
In-Reply-To: <1721159119.197038.1362660611167.JavaMail.root@openwide.fr>
On Thu, Mar 7, 2013 at 12:50 PM, Jeremy Rosen <jeremy.rosen@openwide.fr> wrote:
>> >>
>> >> Git subtree ignores tags from the remote repo.
>> >>
>> >
>> > is that a design decision or a case of "not implemented yet"
>>
>> I'm not sure. If you imported all the tags from all your subtrees
>> repos, you could easily end up with duplicate tags from different
>> repos. They could be namespaced, but there is no concept of namespace
>> in git-subtree. That even assumes that you can tag a subtree (I've
>> not
>> tried).
>>
>
> Ok, I can understand that you don't want to import tags for namespace reason, but in that case shouldn't
> git subtree add refuse to create a subtree when the tag isn't a commit
It shouldn't and tries not to, but is limited in it's ability to
identify if a refspec points to a commit or not in the remote repo.
> or if it allows it, what would be the gracefull way to handle that ?
I've posted a patch (which is pending a lot of other changes to
git-subtree that I'm corralling) that tries to prevent some obvious
errors in the refspec. But letting the git fetch used by git-subtree
add and git-subtree pull catch the error and report it may be the best
option.
> i'm quite new to git's internals, so I don't really know if/what the right approch would be.
>
> note that all those problems seems to disapear when squash is not used
I've never really tried using --squash, I don't see that it adds any
value for me.
--
Paul [W] Campbell
^ permalink raw reply
* Re: Questions/investigations on git-subtree and tags
From: Jeremy Rosen @ 2013-03-07 15:15 UTC (permalink / raw)
To: Paul Campbell; +Cc: git
In-Reply-To: <CALeLG_=xzL1SA0G63FGb5v8K5WQOUg4yaodNm6zwf7kU1pAYvg@mail.gmail.com>
> >
> > Ok, I can understand that you don't want to import tags for
> > namespace reason, but in that case shouldn't
> > git subtree add refuse to create a subtree when the tag isn't a
> > commit
>
> It shouldn't and tries not to, but is limited in it's ability to
> identify if a refspec points to a commit or not in the remote repo.
>
ok, i've studied a little more
* the target for "git subtree add <url> <refspec> can only be a remote branch or tag, since we git fetch
can only target remote refs.
* in case of a branch, git subtree forgets the branch and only use the commit linked to the branch. for
tags, the fetch part is ok, it's the merge part that fail. adding ^{} at the right place would probably fix that
>
> I've posted a patch (which is pending a lot of other changes to
> git-subtree that I'm corralling) that tries to prevent some obvious
> errors in the refspec. But letting the git fetch used by git-subtree
> add and git-subtree pull catch the error and report it may be the
> best
> option.
>
that's interesting... do you have a link ?
>
> I've never really tried using --squash, I don't see that it adds any
> value for me.
>
my project has a git subtree for a linux kernel and another subtree for buildroot,
a default .git is about 1.5G, squashing it reduces it to 200M so it's worth it for me :)
^ permalink raw reply
* Re: Questions/investigations on git-subtree and tags
From: Paul Campbell @ 2013-03-07 15:29 UTC (permalink / raw)
To: Jeremy Rosen; +Cc: git
In-Reply-To: <757358726.204239.1362669338484.JavaMail.root@openwide.fr>
On Thu, Mar 7, 2013 at 3:15 PM, Jeremy Rosen <jeremy.rosen@openwide.fr> wrote:
>> >
>> > Ok, I can understand that you don't want to import tags for
>> > namespace reason, but in that case shouldn't
>> > git subtree add refuse to create a subtree when the tag isn't a
>> > commit
>>
>> It shouldn't and tries not to, but is limited in it's ability to
>> identify if a refspec points to a commit or not in the remote repo.
>>
>
> ok, i've studied a little more
>
> * the target for "git subtree add <url> <refspec> can only be a remote branch or tag, since we git fetch
> can only target remote refs.
> * in case of a branch, git subtree forgets the branch and only use the commit linked to the branch. for
> tags, the fetch part is ok, it's the merge part that fail. adding ^{} at the right place would probably fix that
I think I tried adding the ^{} syntax, but I don't think it works on
remote repos. Or I couldn't get the right syntax.
>>
>> I've posted a patch (which is pending a lot of other changes to
>> git-subtree that I'm corralling) that tries to prevent some obvious
>> errors in the refspec. But letting the git fetch used by git-subtree
>> add and git-subtree pull catch the error and report it may be the
>> best
>> option.
>>
>
> that's interesting... do you have a link ?
Latest patch:
http://thread.gmane.org/gmane.comp.version-control.git/217257
Prior patch with comments from Junio on what was probably going on
with the old tests:
http://thread.gmane.org/gmane.comp.version-control.git/217227
>>
>> I've never really tried using --squash, I don't see that it adds any
>> value for me.
>>
>
> my project has a git subtree for a linux kernel and another subtree for buildroot,
>
> a default .git is about 1.5G, squashing it reduces it to 200M so it's worth it for me :)
If disk space is the issue, or bandwidth for initial cloning, then
sure, but I thought Git was efficient enough that a large repo
wouldn't give much of a performance hit. Unless you use git-subtree
split or push, they are slow.
If git-subtree split could be optimised then --squash wouldn't be
needed as much. It does take an age compared to other Git operations.
--
Paul [W] Campbell
^ permalink raw reply
* Re: Questions/investigations on git-subtree and tags
From: Jeremy Rosen @ 2013-03-07 16:09 UTC (permalink / raw)
To: Paul Campbell; +Cc: git
In-Reply-To: <CALeLG_noUfcOZ8gUjqftz8sfWiWdXP3kZUjkRNJ4W=_J+V70rw@mail.gmail.com>
>
> I think I tried adding the ^{} syntax, but I don't think it works on
> remote repos. Or I couldn't get the right syntax.
>
indeed, it doesn't work on fetch, but it could be used somewhere between the fetch and the commit-tree to move from the ref to the associated commit
>
> Latest patch:
>
> http://thread.gmane.org/gmane.comp.version-control.git/217257
>
oh, that patch, yes I found it while looking around it is a step in the right direction but it doesn't help in my case since i'm using a valid remote ref that can be fetched
(on a side note you could use git ls-remote to check for the remote ref and avoid a fetch in case of an incorrect ref, but that's just a detail)
I just tested with it and here is what happens
git subtree add --squash -P br2 git://git.buildroot.net/buildroot 2013.02 => works ok, br2 is created
however the message of the squash commit is
Squashed 'br2/' content from commit f1d2c19
git-subtree-dir: br2
git-subtree-split: f1d2c19091e1c2ef803ec3267fe71cf6ce7dd948
which is not correct :
git ls-remote git://git.buildroot.net/buildroot 2013.02
f1d2c19091e1c2ef803ec3267fe71cf6ce7dd948 refs/tags/2013.02
git ls-remote git://git.buildroot.net/buildroot 2013.02^{}
15ace1a845c9e7fc65b648bbaf4dd14e03d938fd refs/tags/2013.02^{}
as you can see git subtee thinks it splited from the tag SHA instead of the tag's commit SHA
this is incorrect because the tag isn't here, and at split time git subtree won't be able to find the correct ancestor. We just need to make sure we use the tag's commit instead
of the tag
changing
revs=FETCH_HEAD
to
revs=FETCH_HEAD^{}
in cmd_add_repository
seems to fix it, both for remote branch pull and remote tag pull
we still have a bug lurking around it's the case where the user does the fetch himself then use subtree add with a tag SHA. but let's discuss problems one at a time :)
^ permalink raw reply
* [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string
From: Andrew Wong @ 2013-03-07 16:36 UTC (permalink / raw)
To: git; +Cc: Andrew Wong
The previous code was assuming length ends at either `)` or `,`, and was
not handling the case where strcspn returns length due to end of string.
So specifying ":(top" as pathspec will cause the loop to go pass the end
of string.
Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
setup.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 1dee47e..f4c4e73 100644
--- a/setup.c
+++ b/setup.c
@@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char
*copyfrom && *copyfrom != ')';
copyfrom = nextat) {
size_t len = strcspn(copyfrom, ",)");
- if (copyfrom[len] == ')')
+ if (copyfrom[len] == '\0')
nextat = copyfrom + len;
- else
+ else if (copyfrom[len] == ')')
+ nextat = copyfrom + len;
+ else if (copyfrom[len] == ',')
nextat = copyfrom + len + 1;
if (!len)
continue;
--
1.8.2.rc0.22.gb3600c3
^ permalink raw reply related
* Re: feature suggestion: optimize common parts for checkout --conflict=diff3
From: Junio C Hamano @ 2013-03-07 17:26 UTC (permalink / raw)
To: Jeff King; +Cc: Antoine Pelisse, Uwe Kleine-König, git, kernel
In-Reply-To: <20130307080411.GA25506@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I was also curious whether it would the diff3/zealous combination would
> trigger any weird corner cases. In particular, I wanted to know how the
> example you gave in that commit of:
>
> postimage#1: 1234ABCDE789
> | /
> | /
> preimage: 123456789
> | \
> | \
> postimage#2: 1234AXCYE789
>
> would react with diff3 (this is not the original example, but one with
> an extra "C" in the middle of postimage#2, which could in theory be
> presented as split hunks). However, it seems that we do not do such hunk
> splitting at all, neither for diff3 nor for the "merge" representation.
Without thinking about it too deeply,...
I think the "RCS merge" _could_ show it as "1234A<B=X>C<D=Y>E789"
without losing any information (as it is already discarding what was
in the original in the part that is affected by the conflict,
i.e. "56 was there").
Let's think aloud how "diff3 -m" _should_ split this. The most
straight-forward representation would be "1234<ABCDE|56=AXCYE>789",
that is, where "56" was originally there, one side made it to
"ABCDE" and the other "AXCYE".
You could make it "1234<AB|5=AX><C|=C><DE|6=YE>789", and that is
technically correct (what there were in the shared original for the
conflicted part is 5 and then 6), but the representation pretends
that it knows more than there actually is information, which may be
somewhat misleading. All these three are equally plausible split of
the original "56":
1234<AB|=AX><C|=C><DE|56=YE>789
1234<AB|5=AX><C|=C><DE|6=YE>789
1234<AB|56=AX><C|=C><DE|=YE>789
and picking one over others would be a mere heuristic. All three
are technically correct representations and it is just the matter of
which one is the easiest to understand. So, this is the kind of
"misleading but not incorrect".
In all these cases, the middle part would look like this:
<<<<<<< ours
C
||||||| base
=======
C
>>>>>>> theirs
in order to honor the explicit "I want to view all three versions to
examine the situation" aka "--conflict=diff3" option. We cannot
reduce it to just "C". That will make it "not just misleading but
is actively wrong".
^ permalink raw reply
* Re: What I want rebase to do
From: Junio C Hamano @ 2013-03-07 17:38 UTC (permalink / raw)
To: Thomas Rast; +Cc: Dale R. Worley, git
In-Reply-To: <87r4jra942.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> I still think that the _right_ solution is first redoing the merge on
> its original parents and then seeing how the actual merge differs from
> that.
I think that is what was suggested in
http://article.gmane.org/gmane.comp.version-control.git/198316
> Perhaps a new option to git-rebase could trigger the above behavior for
> merges, who knows. (It could be called --first-parent.)
Yeah, I think that is what the old thread concluded to be the way to
move forward:
http://thread.gmane.org/gmane.comp.version-control.git/198125
I'll throw it in to the "leftover bits".
http://git-blame.blogspot.com/2013/02/more-leftover-bits.html
^ permalink raw reply
* Re: [PATCH] add: Clarify documentation of -A and -u
From: Junio C Hamano @ 2013-03-07 17:52 UTC (permalink / raw)
To: Greg Price; +Cc: git
In-Reply-To: <20130307101314.GP22203@biohazard-cafe.mit.edu>
Greg Price <price@MIT.EDU> writes:
> On Wed, Mar 06, 2013 at 09:10:57AM -0800, Junio C Hamano wrote:
>> I do not know if mentioning what -A does in the description for -u
>> (and vice versa) makes it easier to understand or more confusing
>> (not rhetorical: I suspect some may find it easier and others not).
>>
>> But "and the default behaviour will..." here _is_ confusing. After
>> reading this patch three times, I still cannot tell what "default"
>> you are trying to describe. Is it "-u" without arguments? Is it
>> "add" without "-u" nor "-A"? Is it something else???
>
> I meant the behavior without -A or -u. This could be fixed directly
> by s/the default behavior will/with neither -A nor -u we/.
When we have bulletted list that enumerates options and describes
what each option does and how each option affects the behaviour, I'd
prefer to see us *not* talking about what happens when you do *not*
give that option, unless it makes it hard to understand that option
without such an extra description. The overall description of what
the command does without the options should go to the top level.
> Here's a crack at making those distinctions clear. I've also tried to
> make the descriptions as parallel as possible, as what they're saying
> is very similar.
>
> -u::
> --update::
> Update the index just where it already has an entry matching
> <pathspec>.
Good; this was the short phrasing I was looking for but couldn't
come up with myself without repeating "the index" over and over.
> Then follow both with the "If no <pathspec>" paragraph. I just
> noticed that the paragraph actually needs a small modification to fit
> '-A', too. New patch below.
>
> Greg
>
> From: Greg Price <price@mit.edu>
> Date: Thu, 7 Mar 2013 02:08:21 -0800
> Subject: [PATCH] add: Clarify documentation of -A and -u
(for future reference) Drop the three lines and have "-- >8 --" here.
[patch kept unsnipped for others']
> The documentation of '-A' and '-u' is very confusing for someone who
> doesn't already know what they do. Describe them with fewer words and
> clearer parallelism to each other and to the behavior of plain 'add'.
>
> Also mention the default <pathspec> for '-A' as well as '-u', because
> it applies to both.
>
> Signed-off-by: Greg Price <price@mit.edu>
> ---
> Documentation/git-add.txt | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 388a225..b0944e5 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -100,12 +100,9 @@ apply to the index. See EDITING PATCHES below.
>
> -u::
> --update::
> - Only match <pathspec> against already tracked files in
> - the index rather than the working tree. That means that it
> - will never stage new files, but that it will stage modified
> - new contents of tracked files and that it will remove files
> - from the index if the corresponding files in the working tree
> - have been removed.
> + Update the index just where it already has an entry matching
> + <pathspec>. This removes as well as modifies index entries to
> + match the working tree, but adds no new files.
> +
> If no <pathspec> is given, the current version of Git defaults to
> "."; in other words, update all tracked files in the current directory
> @@ -114,10 +111,15 @@ of Git, hence the form without <pathspec> should not be used.
>
> -A::
> --all::
> - Like `-u`, but match <pathspec> against files in the
> - working tree in addition to the index. That means that it
> - will find new files as well as staging modified content and
> - removing files that are no longer in the working tree.
> + Update the index not only where the working tree has a file
> + matching <pathspec> but also where the index already has an
> + entry. This adds, modifies, and removes index entries to
> + match the working tree.
> ++
> +If no <pathspec> is given, the current version of Git defaults to
> +"."; in other words, update all files in the current directory
> +and its subdirectories. This default will change in a future version
> +of Git, hence the form without <pathspec> should not be used.
>
> -N::
> --intent-to-add::
^ permalink raw reply
* Re: feature suggestion: optimize common parts for checkout --conflict=diff3
From: Jeff King @ 2013-03-07 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Antoine Pelisse, Uwe Kleine-König, git, kernel
In-Reply-To: <7v1ubrnmtu.fsf@alter.siamese.dyndns.org>
On Thu, Mar 07, 2013 at 09:26:05AM -0800, Junio C Hamano wrote:
> Without thinking about it too deeply,...
>
> I think the "RCS merge" _could_ show it as "1234A<B=X>C<D=Y>E789"
> without losing any information (as it is already discarding what was
> in the original in the part that is affected by the conflict,
> i.e. "56 was there").
Right, I think that is sane, though we do not do that at this point.
> Let's think aloud how "diff3 -m" _should_ split this. The most
> straight-forward representation would be "1234<ABCDE|56=AXCYE>789",
> that is, where "56" was originally there, one side made it to
> "ABCDE" and the other "AXCYE".
Yes, that is what diff3 would do now (because it does not do any hunk
refinement at all), and should continue doing.
> You could make it "1234<AB|5=AX><C|=C><DE|6=YE>789", and that is
> technically correct (what there were in the shared original for the
> conflicted part is 5 and then 6), but the representation pretends
> that it knows more than there actually is information, which may be
> somewhat misleading. All these three are equally plausible split of
> the original "56":
>
> 1234<AB|=AX><C|=C><DE|56=YE>789
> 1234<AB|5=AX><C|=C><DE|6=YE>789
> 1234<AB|56=AX><C|=C><DE|=YE>789
>
> and picking one over others would be a mere heuristic. All three
> are technically correct representations and it is just the matter of
> which one is the easiest to understand. So, this is the kind of
> "misleading but not incorrect".
Yes, I agree it is a heuristic about which part of a split hunk to place
deleted preimage lines in. Conceptually, I'm OK with that; the point of
zdiff3 is to try to make the conflict easier to read by eliminating
possibly uninteresting parts. It doesn't have to be right all the time;
it just has to be useful most of the time. But it's not clear how true
that would be in real life.
I think this is somewhat a moot point, though. We do not do this
splitting now. If we later learn to do it, there is nothing to say that
zdiff3 would have to adopt it also; it could stop at a lower
zealous-level than the regular merge markers. I think I'd want to
experiment with it and see some real-world examples before making a
decision on that.
> In all these cases, the middle part would look like this:
>
> <<<<<<< ours
> C
> ||||||| base
> =======
> C
> >>>>>>> theirs
>
> in order to honor the explicit "I want to view all three versions to
> examine the situation" aka "--conflict=diff3" option. We cannot
> reduce it to just "C". That will make it "not just misleading but
> is actively wrong".
I'm not sure I agree. In this output (which does the zealous
simplification, the splitting, and arbitrarily assigns deleted preimage
to the first of the split hunks):
1234A<B|56=X>C<D|Y>E789
I do not see the promotion of C to "already resolved, you cannot tell if
it was really in the preimage or not" as any more or less misleading or
wrong than that of A or E. It is no more misleading than what the
merge-marker case would do, which would be:
1234A<B=X>C<D=Y>E789
The wrong thing to me is the arbitrary choice about how to distribute
the preimage lines. In this example, it is not a big deal for the
heuristic to be wrong; you can see both of the hunks. But if C is long,
and you do not even see D=Y while resolving B=X, seeing the preimage
there may become nonsensical.
But again, we don't do this splitting now. So I don't think it's
something that should make or break a decision to have zdiff3. Without
the splitting, I can see it being quite useful. I'm going to carry the
patch in my tree for a while and try using it in practice for a while.
-Peff
^ permalink raw reply
* Re: feature suggestion: optimize common parts for checkout --conflict=diff3
From: Junio C Hamano @ 2013-03-07 18:21 UTC (permalink / raw)
To: Jeff King; +Cc: Antoine Pelisse, Uwe Kleine-König, git, kernel
In-Reply-To: <7v1ubrnmtu.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> You could make it "1234<AB|5=AX><C|=C><DE|6=YE>789", and that is
> technically correct (what there were in the shared original for the
> conflicted part is 5 and then 6), but the representation pretends
> that it knows more than there actually is information, which may be
> somewhat misleading. All these three are equally plausible split of
> the original "56":
>
> 1234<AB|=AX><C|=C><DE|56=YE>789
> 1234<AB|5=AX><C|=C><DE|6=YE>789
> 1234<AB|56=AX><C|=C><DE|=YE>789
>
> and picking one over others would be a mere heuristic. All three
> are technically correct representations and it is just the matter of
> which one is the easiest to understand. So, this is the kind of
> "misleading but not incorrect".
I forgot to say that youu could even do something silly like:
1234<AB|=AX><C|56=C><DE|=YE>789
;-)
> In all these cases, the middle part would look like this:
>
> <<<<<<< ours
> C
> ||||||| base
> =======
> C
> >>>>>>> theirs
>
> in order to honor the explicit "I want to view all three versions to
> examine the situation" aka "--conflict=diff3" option. We cannot
> reduce it to just "C". That will make it "not just misleading but
> is actively wrong".
I also forgot to say that the issue is the same to reduce
1234<AB|=AX><C|=C><DE|56=YE>789
to
1234<A|=A><B|=X><C|=C><D|56=Y><E|=E>789
which is unconditionally correct and then for all x reduce <x|=x> to
x, yielding
1234A<B|=X>C<D|56=Y>E789
which your zealous-diff3 would do. So squashing that <C|=C> in the
middle would be consistent if you take the zealous-diff3 route.
But again, that is discarding the information of the original, which
the user explicitly asked from "diff3 -m", i.e. show all three to
examine the situation. If the user wants to operate _without_ the
original, the user would have asked for "RCS merge" style output, so
I am still not sure if that is a sensible mode of operation for diff3
to begin with.
^ permalink raw reply
* Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool
From: Kevin Bracey @ 2013-03-07 17:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Ciaran Jessup, Scott Chacon
In-Reply-To: <7vd2vboepi.fsf@alter.siamese.dyndns.org>
On 07/03/2013 09:23, Junio C Hamano wrote:
> If p4merge GUI labels one side clearly as "theirs" and the other
> "ours", and the way we feed the inputs to it makes the side that is
> actually "ours" appear in p4merge GUI labelled as "theirs", then I do
> not think backward compatibility argument does not hold water. It is
> just correcting a longstanding 3-4 year old bug in a tool that nobody
> noticed.
It's not quite that clear-cut. Some years ago, and before p4merge was
added as a Git mergetool, P4Merge was changed so its main GUI text says
"left" and "right" instead of "theirs" and "ours" when invoked manually.
But it appears that's as far as they went. It doesn't seem any of its
asymmetric diff display logic was changed; it works better with ours on
the right, and the built-in help all remains written on the theirs/ours
basis. And even little details like the icons imply it (a square for the
base, a downward-pointing triangle for their incoming stuff, and a
circle for the version we hold).
> For people who are very used to the way p4merge shows the merged
> contents by theirs-base-yours order in side-by-side view, I do not
> think it is unreasonable to introduce the "mergetool.$name.reverse"
> configuration variable and teach the mergetool frontend to pay
> attention to it. That will allow them to see their merge in reverse
> order even when they are using a backend other than p4merge.
>
> With such a mechanism in place, by default, we can just declare that
> mergetool.p4merge.reverse is "true" when unset, while making
> mergetool.$name.reverse for all the other tools default to "false".
> People who are already used to the way our p4merge integration works
> can set mergetool.p4merge.reverse to "false" explicitly to retain
> the historical behaviour that you are declaring "buggy" with such a
> change.
I like this idea as a user - having made this change to p4merge, it does
throw me when I decide to attempt a particularly tricky merge with bc3
instead, and get the other order. The user config options you suggest
sound good to me.
For completion on this idea, I'd suggest difftool.xxx.reverse, to allow
the orientation for 0- and 1-revision diffs to be chosen - allow the
implied working tree version to be on the left or right. That would
allow "ours-theirs" order, which some would view as being more
consistent with the "ours-base-theirs" default for mergetool.
Would it be going too far to also have "xxxtool.reverse" to choose the
global default? Then the choice hierarchy would be "xxxtool.xxx.reverse
if set" > "optional inbuilt tool preference" > "xxxtool.reverse if set"
> "false". So the user could request a global swap, except that they'd
have to explicitly override any tools that have a preferred orientation.
My only reservation is that I assume it would be implemented by swapping
what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
$LOCAL="a.REMOTE.1234.c". On the other hand, $LOCAL and $REMOTE are
already not very meaningful names for difftool... Maybe we should change
to using $LEFT and $RIGHT, acknowledging the existing difftool
situation, and that the user can now swap merges too.
I'd be happy to prepare a fuller patch on this sort of basis.
Kevin
^ permalink raw reply
* [PATCH] Fix revision walk for commits with the same dates
From: Kacper Kornet @ 2013-03-07 18:03 UTC (permalink / raw)
To: git
git rev-list A^! --not B provides wrong answer if all commits in the
range A..B had the same commit times and there are more then 8 of them.
This commits fixes the logic in still_interesting function to prevent
this error.
Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
revision.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index ef60205..cf620c6 100644
--- a/revision.c
+++ b/revision.c
@@ -709,7 +709,7 @@ static int still_interesting(struct commit_list *src, unsigned long date, int sl
* Does the destination list contain entries with a date
* before the source list? Definitely _not_ done.
*/
- if (date < src->item->date)
+ if (date <= src->item->date)
return SLOP;
/*
--
1.8.2.rc2
^ permalink raw reply related
* Re: feature suggestion: optimize common parts for checkout --conflict=diff3
From: Junio C Hamano @ 2013-03-07 18:40 UTC (permalink / raw)
To: Jeff King; +Cc: Antoine Pelisse, Uwe Kleine-König, git, kernel
In-Reply-To: <20130307180157.GA6604@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm not sure I agree. In this output (which does the zealous
> simplification, the splitting, and arbitrarily assigns deleted preimage
> to the first of the split hunks):
>
> 1234A<B|56=X>C<D|Y>E789
>
> I do not see the promotion of C to "already resolved, you cannot tell if
> it was really in the preimage or not" as any more or less misleading or
> wrong than that of A or E. It is no more misleading than what the
> merge-marker case would do, which would be:
>
> 1234A<B=X>C<D=Y>E789
That is exactly my point and I think we are in complete agreement.
While the intended difference between RCS merge and diff3 -m is for
the latter not to lose information on the original, zealous-diff3
chooses to lose information in "both sides added, identically" case.
Where we differ is if such information loss is a good thing to have.
We could say "both sides added, identically" is auto-resolved when
you use the zealous option, and do so regardless of how the merge
conflicts are presented. Then it becomes perfectly fine to eject
"A" and "E" out of the conflicted block and merge them to be part of
pre/post contexts. The same goes for reducing "<C|=C>" to "C". As
long as we clearly present the users what the option does and what
its implications are, it is not bad to have such an option, I think.
> The wrong thing to me is the arbitrary choice about how to distribute
> the preimage lines.
Yeah, but that is not "diff3 -m" vs "zealous-diff3" issue, is it?
If you value the original and want to show it somewhere, you cannot
avoid making the choice whether you are zealous or not if you split
such a hunk.
^ permalink raw reply
* [PATCH] line-log: Fix sparse warnings
From: Ramsay Jones @ 2013-03-07 18:27 UTC (permalink / raw)
To: trast; +Cc: Junio C Hamano, GIT Mailing-list
Sparse issues the following warnings:
line-log.c:17:6: warning: symbol 'range_set_grow' was not declared. Should it be static?
line-log.c:25:6: warning: symbol 'range_set_init' was not declared. Should it be static?
line-log.c:33:6: warning: symbol 'range_set_release' was not declared. Should it be static?
line-log.c:41:6: warning: symbol 'range_set_copy' was not declared. Should it be static?
line-log.c:47:6: warning: symbol 'range_set_move' was not declared. Should it be static?
line-log.c:58:6: warning: symbol 'range_set_append' was not declared. Should it be static?
line-log.c:299:46: warning: Using plain integer as NULL pointer
line-log.c:444:12: warning: symbol 'parse_loc' was not declared. Should it be static?
line-log.c:681:49: warning: Using plain integer as NULL pointer
line-log.c:684:58: warning: Using plain integer as NULL pointer
builtin/log.c:120:57: warning: Using plain integer as NULL pointer
builtin/log.c:120:60: warning: Using plain integer as NULL pointer
In order to suppress the "... was not declared" warnings, we simply
add the static modifier to the declarations of those symbols, since
they do not need more than file scope.
In order to suppress the "NULL pointer" warnings, we simply replace
the use of the integer constant zero as a representation of the null
pointer with the NULL symbol.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Thomas,
If you need to re-roll the patches in your 'tr/line-log' branch, could
you please squash these changes in. [Note that this patch applies to the
tip of the pu branch ;-) ]
Also, I noticed some things in passing, viz:
971 static void load_tree_desc(struct tree_desc *desc, void **tree,
972 const unsigned char *sha1)
973 {
974 unsigned long size;
975 *tree = read_object_with_reference(sha1, tree_type, &size, NULL);
976 if (!tree)
977 die("Unable to read tree (%s)", sha1_to_hex(sha1));
978 init_tree_desc(desc, *tree, size);
979 }
The die() on line 977 will never trigger, since (given that !tree
holds) we will already have dumped core on line 975! ;-)
1401 int line_log_filter(struct rev_info *rev)
1402 {
1403 struct commit *commit;
1404 struct commit_list *list = rev->commits;
1405 struct commit_list *out = NULL, *cur = NULL;
1406
1407 list = rev->commits;
1408 while (list) {
Note that the assignment on line 1407 is redundant and can be
removed; list has been initialized to the same value in it's
declaration on line 1404.
HTH
ATB,
Ramsay Jones
builtin/log.c | 2 +-
line-log.c | 20 ++++++++++----------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index c5d2313..a551d8d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -117,7 +117,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
{
struct userformat_want w;
int quiet = 0, source = 0, mailmap = 0;
- static struct line_opt_callback_data line_cb = {0, 0, STRING_LIST_INIT_DUP};
+ static struct line_opt_callback_data line_cb = {NULL, NULL, STRING_LIST_INIT_DUP};
const struct option builtin_log_options[] = {
OPT_BOOLEAN(0, "quiet", &quiet, N_("suppress diff output")),
diff --git a/line-log.c b/line-log.c
index a74bbaf..4d01798 100644
--- a/line-log.c
+++ b/line-log.c
@@ -14,7 +14,7 @@
#include "userdiff.h"
#include "line-log.h"
-void range_set_grow (struct range_set *rs, size_t extra)
+static void range_set_grow (struct range_set *rs, size_t extra)
{
ALLOC_GROW(rs->ranges, rs->nr + extra, rs->alloc);
}
@@ -22,7 +22,7 @@ void range_set_grow (struct range_set *rs, size_t extra)
/* Either initialization would be fine */
#define RANGE_SET_INIT {0}
-void range_set_init (struct range_set *rs, size_t prealloc)
+static void range_set_init (struct range_set *rs, size_t prealloc)
{
rs->alloc = rs->nr = 0;
rs->ranges = NULL;
@@ -30,7 +30,7 @@ void range_set_init (struct range_set *rs, size_t prealloc)
range_set_grow(rs, prealloc);
}
-void range_set_release (struct range_set *rs)
+static void range_set_release (struct range_set *rs)
{
free(rs->ranges);
rs->alloc = rs->nr = 0;
@@ -38,13 +38,13 @@ void range_set_release (struct range_set *rs)
}
/* dst must be uninitialized! */
-void range_set_copy (struct range_set *dst, struct range_set *src)
+static void range_set_copy (struct range_set *dst, struct range_set *src)
{
range_set_init(dst, src->nr);
memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
dst->nr = src->nr;
}
-void range_set_move (struct range_set *dst, struct range_set *src)
+static void range_set_move (struct range_set *dst, struct range_set *src)
{
range_set_release(dst);
dst->ranges = src->ranges;
@@ -55,7 +55,7 @@ void range_set_move (struct range_set *dst, struct range_set *src)
}
/* tack on a _new_ range _at the end_ */
-void range_set_append (struct range_set *rs, long a, long b)
+static void range_set_append (struct range_set *rs, long a, long b)
{
assert(a <= b);
assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
@@ -296,7 +296,7 @@ static int collect_diff_cb (long start_a, long count_a,
static void collect_diff (mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
{
- struct collect_diff_cbdata cbdata = {0};
+ struct collect_diff_cbdata cbdata = {NULL};
xpparam_t xpp;
xdemitconf_t xecfg;
xdemitcb_t ecb;
@@ -441,7 +441,7 @@ static void range_set_map_across_diff (struct range_set *out,
/*
* Parse one item in the -L option
*/
-const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
+static const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
void *data, long lines, long begin, long *ret)
{
char *term;
@@ -678,10 +678,10 @@ const char *skip_range_arg(const char *arg)
if (*arg == ':')
return parse_range_funcname(arg, NULL, NULL, 0, NULL, NULL, NULL);
- arg = parse_loc(arg, NULL, NULL, 0, -1, 0);
+ arg = parse_loc(arg, NULL, NULL, 0, -1, NULL);
if (*arg == ',')
- arg = parse_loc(arg+1, NULL, NULL, 0, 0, 0);
+ arg = parse_loc(arg+1, NULL, NULL, 0, 0, NULL);
return arg;
}
--
1.8.1
^ permalink raw reply related
* Re: [PATCH 4/4] teach config parsing to read from strbuf
From: Ramsay Jones @ 2013-03-07 18:42 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Jeff King
In-Reply-To: <20130226194313.GE22756@sandbox-ub>
Heiko Voigt wrote:
> This can be used to read configuration values directly from gits
> database.
>
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
> .gitignore | 1 +
> Makefile | 1 +
> cache.h | 1 +
> config.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> t/t1300-repo-config.sh | 4 ++++
> test-config.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 95 insertions(+)
> create mode 100644 test-config.c
>
[...]
> diff --git a/config.c b/config.c
> index 19aa205..492873a 100644
> --- a/config.c
> +++ b/config.c
> @@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)
> return ftell(f);
> }
>
> +struct config_strbuf {
> + struct strbuf *strbuf;
> + int pos;
> +};
> +
> +static int config_strbuf_fgetc(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos < str->strbuf->len)
> + return str->strbuf->buf[str->pos++];
> +
> + return EOF;
> +}
> +
> +static int config_strbuf_ungetc(int c, struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos > 0)
> + return str->strbuf->buf[--str->pos];
> +
> + return EOF;
> +}
> +
> +static long config_strbuf_ftell(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> + return str->pos;
> +}
> +
> #define MAX_INCLUDE_DEPTH 10
> static const char include_depth_advice[] =
> "exceeded maximum include depth (%d) while including\n"
> @@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> return ret;
> }
>
> +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
> +{
> + struct config top;
> + struct config_strbuf str;
> +
> + str.strbuf = strbuf;
> + str.pos = 0;
> +
> + top.data = &str;
You will definitely want to initialise 'top.name' here, rather
than let it take whatever value happens to be at that position
on the stack. In your editor, search for 'cf->name' and contemplate
each such occurrence.
> + top.fgetc = config_strbuf_fgetc;
> + top.ungetc = config_strbuf_ungetc;
> + top.ftell = config_strbuf_ftell;
> +
> + return do_config_from(&top, fn, data);
> +}
> +
> const char *git_etc_gitconfig(void)
> {
> static const char *system_wide;
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 3c96fda..3304bcd 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '
> grep " line 3 " error
> '
>
> +test_expect_success 'reading config from strbuf' '
> + test-config strbuf
> +'
> +
> test_done
> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 0000000..7a4103c
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,41 @@
> +#include "cache.h"
> +
> +static const char *config_string = "[some]\n"
> + " value = content\n";
> +
> +static int config_strbuf(const char *var, const char *value, void *data)
> +{
> + int *success = data;
> + if (!strcmp(var, "some.value") && !strcmp(value, "content"))
> + *success = 0;
> +
> + printf("var: %s, value: %s\n", var, value);
> +
> + return 1;
> +}
> +
> +static void die_usage(int argc, char **argv)
> +{
> + fprintf(stderr, "Usage: %s strbuf\n", argv[0]);
> + exit(1);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + if (argc < 2)
> + die_usage(argc, argv);
> +
> + if (!strcmp(argv[1], "strbuf")) {
> + int success = 1;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, config_string);
> + git_config_from_strbuf(config_strbuf, &buf, &success);
> +
> + return success;
> + }
> +
> + die_usage(argc, argv);
> +
> + return 1;
> +}
>
Does the 'include' facility work from a strbuf? Should it?
Are you happy with the error handling/reporting?
Do the above additions to the test-suite give you confidence
that the code works as you intend?
ATB,
Ramsay Jones
^ permalink raw reply
* Re: feature suggestion: optimize common parts for checkout --conflict=diff3
From: Jeff King @ 2013-03-07 18:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Antoine Pelisse, Uwe Kleine-König, git, kernel
In-Reply-To: <7vfw07m4sx.fsf@alter.siamese.dyndns.org>
On Thu, Mar 07, 2013 at 10:40:46AM -0800, Junio C Hamano wrote:
> Where we differ is if such information loss is a good thing to have.
>
> We could say "both sides added, identically" is auto-resolved when
> you use the zealous option, and do so regardless of how the merge
> conflicts are presented. Then it becomes perfectly fine to eject
> "A" and "E" out of the conflicted block and merge them to be part of
> pre/post contexts. The same goes for reducing "<C|=C>" to "C". As
> long as we clearly present the users what the option does and what
> its implications are, it is not bad to have such an option, I think.
Exactly. I do think it has real-world uses (see the example script I
posted yesterday), but it would never replace diff3. I'm going to try it
out for a bit. As I mentioned yesterday, I see those sorts of
cherry-pick-with-something-on-top conflicts when I am rebasing onto or
merging my topics into what you have picked up from the same topic on
the list.
I think the code in Uwe's patch looked fine, but it definitely needs a
documentation change to explain the new mode and its caveats. I'd also
be happy with a different name, if you think it implies that it is too
related to zdiff3, but I cannot think of anything better at the moment.
> > The wrong thing to me is the arbitrary choice about how to distribute
> > the preimage lines.
>
> Yeah, but that is not "diff3 -m" vs "zealous-diff3" issue, is it?
> If you value the original and want to show it somewhere, you cannot
> avoid making the choice whether you are zealous or not if you split
> such a hunk.
Right, but I meant that we would never split a hunk like that with
diff3, because we would not do any hunk refinement at all. Splitting a
hunk with "merge" is OK, because the "where does the preimage go"
problem does not exist there. zdiff3 is the only problematic case,
because it would be the only one that (potentially) splits and cares
about how the preimage maps to each hunk. But we can deal with that if
and when we ever do such splitting.
-Peff
^ permalink raw reply
* Re: Merging submodules - best merge-base
From: Heiko Voigt @ 2013-03-07 18:59 UTC (permalink / raw)
To: Daniel Bratell; +Cc: git, Jens Lehmann
In-Reply-To: <op.wtklj7e9rbppqq@cicero.linkoping.osa>
On Thu, Mar 07, 2013 at 10:49:09AM +0100, Daniel Bratell wrote:
> Den 2013-03-06 19:12:05 skrev Heiko Voigt <hvoigt@hvoigt.net>:
>
> >On Mon, Feb 25, 2013 at 05:44:05PM +0100, Daniel Bratell wrote:
> >>A submodule change can be merged, but only if the merge is a
> >>"fast-forward" which I think is a fair demand, but currently it
> >>checks if
> >>it's a fast-forward from a commit that might not be very interesting
> >>anymore.
> >>
> >>If two branches A and B split at a point when they used submodule commit
> >>S1 (based on S), and both then switched to S2 (also based on S)
> >>and B then
> >>switched to S21, then it's today not possible to merge B into A, despite
> >>S21 being a descendant of S2 and you get a conflict and this warning:
> >>
> >>warning: Failed to merge submodule S (commits don't follow merge-base)
> >>
> >>(attempt at ASCII gfx:
> >>
> >>Submodule tree:
> >>
> >>S ---- S1
> >> \
> >> \ - S2 -- S21
> >>
> >>Main tree:
> >>
> >>A' (uses S1) --- A (uses S2)
> >> \
> >> \ --- B' (uses S2) -- B (uses S21)
> >>
> >>
> >>I would like it to end up as:
> >>
> >>A' (uses S1) --- A (uses S2) ------------ A+ (uses S21)
> >> \ /
> >> \ --- B' (uses S2) -- B (uses S21)- /
> >>
> >>And that should be legal since S21 is a descendant of S2.
> >
> >So to summarize what you are requesting: You want a submodule merge be
> >two way in the view of the superproject and calculate the merge base
> >in the submodule from the two commits that are going to be merged?
> >
> >It currently sounds logical but I have to think about it further and
> >whether that might break other use cases.
>
> Maybe both could be legal even. The current code can't be all wrong,
> and this case also seems to be straightforward.
Ok I have thought about it further and I did not come up with a simple
(and stable) enough strategy that would allow your use case to merge
cleanly without user interaction.
The problem is that your are actually doing a rewind from base to both
tips. The fact that a rewind is there makes git suspicious and we simply
give up. IMO, thats the right thing to do in such a situation.
What should a merge strategy do? It infers from two changes what the
final intention might be. For submodules we can do that when the changes
on both sides point forward. Since thats the typical progress of
development. If not there is some reason for it we do not know about. So
the merge gives up.
Please see this post about why we need to forbid rewinds from the
initial design discussion:
http://article.gmane.org/gmane.comp.version-control.git/149003
I am not saying that the current behavior is perfect but I think a merge
containing a rewind needs user support. We could give the user a hint
and say: "Hey I gave up but the two sides are contained in each other
and this is the commit containing both". Then the user can choose to use
that suggested solution. We already do the same for the merge commit
search.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool
From: Junio C Hamano @ 2013-03-07 19:10 UTC (permalink / raw)
To: Kevin Bracey; +Cc: git, David Aguilar, Ciaran Jessup, Scott Chacon
In-Reply-To: <5138CAFE.2010602@bracey.fi>
Kevin Bracey <kevin@bracey.fi> writes:
> On 07/03/2013 09:23, Junio C Hamano wrote:
>> If p4merge GUI labels one side clearly as "theirs" and the other
>> "ours", and the way we feed the inputs to it makes the side that is
>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>> do not think backward compatibility argument does not hold water. It
>> is just correcting a longstanding 3-4 year old bug in a tool that
>> nobody noticed.
>
> It's not quite that clear-cut. Some years ago, and before p4merge was
> added as a Git mergetool, P4Merge was changed so its main GUI text
> says "left" and "right" instead of "theirs" and "ours" when invoked
> manually.
>
> But it appears that's as far as they went. It doesn't seem any of its
> asymmetric diff display logic was changed; it works better with ours
> on the right, and the built-in help all remains written on the
> theirs/ours basis. And even little details like the icons imply it (a
> square for the base, a downward-pointing triangle for their incoming
> stuff, and a circle for the version we hold).
So in short, a user of p4merge can see that left side is intended as
"theirs", even though recent p4merge sometimes calls it "left". And
your description on the coloring (green vs blue) makes it clear that
"left" and "theirs" are still intended to be synonyms.
If that is the case I would think you can still argue such a change
as "correcting a 3-4-year old bug".
> Would it be going too far to also have "xxxtool.reverse" to choose the
> global default?
It would be a natural thing to do. I left it out because I thought
it would go without saying, given that precedences already exist,
e.g. mergetool.keepBackup etc.
> My only reservation is that I assume it would be implemented by
> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
> $LOCAL="a.REMOTE.1234.c".
Doesn't the UI show the actual temporary filename? When merging my
version of hello.c with your version, showing them as hello.LOCAL.c
and hello.REMOTE.c is an integral part of the UI experience, I
think, even if the GUI tool does not give its own labels (and
behaviour differences as you mentioned for p4merge) to mark which
side is theirs and which side is ours. The temporary file that
holds their version should still be named with REMOTE, even when the
mergetool.reverse option is in effect.
As to the name of the variable, I do not care too deeply about it
myself, but I think keeping the current LOCAL and REMOTE would help
people following the code, especially given the option is called
"reverse", meaning that there is an internal convention that the
order is "LOCAL and then REMOTE".
One thing to watch out for is from which temporary file we take the
merged results. You can present the two sides swapped, but if the
tool always writes the results out by updating the second file, the
caller needs to be prepared to read from the one that gets changed.
^ permalink raw reply
* Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool
From: David Aguilar @ 2013-03-07 19:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Bracey, git, Ciaran Jessup, Scott Chacon
In-Reply-To: <7vboavm3fh.fsf@alter.siamese.dyndns.org>
On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> On 07/03/2013 09:23, Junio C Hamano wrote:
>>> If p4merge GUI labels one side clearly as "theirs" and the other
>>> "ours", and the way we feed the inputs to it makes the side that is
>>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>>> do not think backward compatibility argument does not hold water. It
>>> is just correcting a longstanding 3-4 year old bug in a tool that
>>> nobody noticed.
>>
>> It's not quite that clear-cut. Some years ago, and before p4merge was
>> added as a Git mergetool, P4Merge was changed so its main GUI text
>> says "left" and "right" instead of "theirs" and "ours" when invoked
>> manually.
>>
>> But it appears that's as far as they went. It doesn't seem any of its
>> asymmetric diff display logic was changed; it works better with ours
>> on the right, and the built-in help all remains written on the
>> theirs/ours basis. And even little details like the icons imply it (a
>> square for the base, a downward-pointing triangle for their incoming
>> stuff, and a circle for the version we hold).
>
> So in short, a user of p4merge can see that left side is intended as
> "theirs", even though recent p4merge sometimes calls it "left". And
> your description on the coloring (green vs blue) makes it clear that
> "left" and "theirs" are still intended to be synonyms.
>
> If that is the case I would think you can still argue such a change
> as "correcting a 3-4-year old bug".
I would prefer to treat this as a bugfix rather than introducing
a new set of configuration knobs if possible. It really does
seem like a correction.
Users that want the traditional behavior can get that by
configuring a custom mergetool.p4merge.cmd, so we're not
completely losing the ability to get at the old behavior.
Users that want to see a reverse diff with difftool can
already say "--reverse", so there's even less reason to
have it there (though I know we're talking about mergetool only).
>> Would it be going too far to also have "xxxtool.reverse" to choose the
>> global default?
>
> It would be a natural thing to do. I left it out because I thought
> it would go without saying, given that precedences already exist,
> e.g. mergetool.keepBackup etc.
Medium NACK. If we can do without configuration all the better.
I would much rather prefer to have the default/mainstream
behavior be the best out-of-the-box sans configuration.
The reasoning behind swapping them for p4merge makes sense
for p4merge only. I don't think we're quite ready to declare
that all the merge tools need to be swapped or that we need a
mechanism for swapping the order.
>> My only reservation is that I assume it would be implemented by
>> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
>> $LOCAL="a.REMOTE.1234.c".
>
> Doesn't the UI show the actual temporary filename? When merging my
> version of hello.c with your version, showing them as hello.LOCAL.c
> and hello.REMOTE.c is an integral part of the UI experience, I
> think, even if the GUI tool does not give its own labels (and
> behaviour differences as you mentioned for p4merge) to mark which
> side is theirs and which side is ours. The temporary file that
> holds their version should still be named with REMOTE, even when the
> mergetool.reverse option is in effect.
>
> As to the name of the variable, I do not care too deeply about it
> myself, but I think keeping the current LOCAL and REMOTE would help
> people following the code, especially given the option is called
> "reverse", meaning that there is an internal convention that the
> order is "LOCAL and then REMOTE".
>
> One thing to watch out for is from which temporary file we take the
> merged results. You can present the two sides swapped, but if the
> tool always writes the results out by updating the second file, the
> caller needs to be prepared to read from the one that gets changed.
--
David
^ permalink raw reply
* Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool
From: Junio C Hamano @ 2013-03-07 20:31 UTC (permalink / raw)
To: David Aguilar; +Cc: Kevin Bracey, git, Ciaran Jessup, Scott Chacon
In-Reply-To: <CAJDDKr5-ttcU48r0-qTfov7q736Rj63rS33fTScSsvx53VG4pA@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> I would prefer to treat this as a bugfix rather than introducing
> a new set of configuration knobs if possible. It really does
> seem like a correction.
>
> Users that want the traditional behavior can get that by
> configuring a custom mergetool.p4merge.cmd, so we're not
> completely losing the ability to get at the old behavior.
>
> Users that want to see a reverse diff with difftool can
> already say "--reverse", so there's even less reason to
> have it there (though I know we're talking about mergetool only).
> ...
> I would much rather prefer to have the default/mainstream
> behavior be the best out-of-the-box sans configuration.
>
> The reasoning behind swapping them for p4merge makes sense
> for p4merge only. I don't think we're quite ready to declare
> that all the merge tools need to be swapped or that we need a
> mechanism for swapping the order.
Thanks for an injection of sanity.
^ permalink raw reply
* Re: Please pull l10n updates for 1.8.2 round 4
From: Junio C Hamano @ 2013-03-07 21:13 UTC (permalink / raw)
To: Jiang Xin; +Cc: Ralf Thielow, Tran Ngoc Quan, Peter Krefting, Git List
In-Reply-To: <CANYiYbEUSc3s1KZ6tyd9Ot68_o2-hxPE=sD8oA0Dgz-_4G8AWA@mail.gmail.com>
Thanks!
^ 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