* Re: [PATCH 22/48] merge-recursive: Fix sorting order and directory change assumptions
From: Elijah Newren @ 2011-08-08 19:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jgfouca
In-Reply-To: <7vhb6jcg68.fsf@alter.siamese.dyndns.org>
On Mon, Jul 18, 2011 at 5:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +static int string_list_df_name_compare(const void *a, const void *b)
>> +{
>> + const struct string_list_item *one = a;
>> + const struct string_list_item *two = b;
>> + /*
>> + * Here we only care that entries for D/F conflicts are
>> + * adjacent, in particular with the file of the D/F conflict
>> + * appearing before files below the corresponding directory.
>> + * The order of the rest of the list is irrelevant for us.
>> + *
>> + * To achieve this, we sort with df_name_compare and provide
>> + * the mode S_IFDIR so that D/F conflicts will sort correctly.
>> + * We use the mode S_IFDIR for everything else for simplicity,
>> + * since in other cases any changes in their order due to
>> + * sorting cause no problems for us.
>> + */
>
> I recall there was an issue of this sorting reported earlier...
Yes, in git-fast-export, though. It was 060df62 (fast-export: Fix
output order of D/F changes 2010-07-09), if you're curious.
^ permalink raw reply
* Re: [PATCH v2] Tolerate zlib deflation with window size < 32Kb
From: Junio C Hamano @ 2011-08-08 19:23 UTC (permalink / raw)
To: roberto.tyley; +Cc: git, Roberto Tyley
In-Reply-To: <1312742773-26373-1-git-send-email-roberto.tyley@gmail.com>
roberto.tyley@gmail.com writes:
> +. ./test-lib.sh
> +
> +assert_blob_equals() {
> + echo -n $2 > expected &&
> + git cat-file -p $1 > actual &&
Needs proper quoting with dq.
> + test_cmp expected actual
> +}
> +
> +test_expect_success setup '
> + cp -R ../t1013/objects .git/
> + git --version
> +'
> +
> +test_expect_success 'read standard-format loose objects' '
> + git cat-file tag 8d4e360d6c70fbd72411991c02a09c442cf7a9fa &&
> + git cat-file commit 6baee0540ea990d9761a3eb9ab183003a71c3696 &&
> + git ls-tree 7a37b887a73791d12d26c0d3e39568a8fb0fa6e8 &&
> + assert_blob_equals "257cc5642cb1a054f08cc83f2d943e56fd3ebe99" "foo\n"
Isn't it unportable to expect "\n" to be kept or expanded by "echo"?
Will squash the following in (and I have another patch on top to
consolidate the definition of $LF in this and other test scripts).
Thanks.
-- >8 --
Subject: [PATCH] fixup! Tolerate zlib deflation with window
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t1013-loose-object-format.sh | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh
index b5ac46d..36b4027 100755
--- a/t/t1013-loose-object-format.sh
+++ b/t/t1013-loose-object-format.sh
@@ -26,10 +26,12 @@ standard format, deflated with 4KB window size: Agit/JGit on Android
'
. ./test-lib.sh
+LF='
+'
assert_blob_equals() {
- echo -n $2 > expected &&
- git cat-file -p $1 > actual &&
+ printf "%s" "$2" >expected &&
+ git cat-file -p "$1" >actual &&
test_cmp expected actual
}
@@ -42,7 +44,7 @@ test_expect_success 'read standard-format loose objects' '
git cat-file tag 8d4e360d6c70fbd72411991c02a09c442cf7a9fa &&
git cat-file commit 6baee0540ea990d9761a3eb9ab183003a71c3696 &&
git ls-tree 7a37b887a73791d12d26c0d3e39568a8fb0fa6e8 &&
- assert_blob_equals "257cc5642cb1a054f08cc83f2d943e56fd3ebe99" "foo\n"
+ assert_blob_equals "257cc5642cb1a054f08cc83f2d943e56fd3ebe99" "foo$LF"
'
test_expect_success 'read experimental-format loose objects' '
--
1.7.6.409.ge7a85
^ permalink raw reply related
* Re: [PATCH] filter-branch: Export variable `workdir' for --commit-filter
From: Junio C Hamano @ 2011-08-08 19:11 UTC (permalink / raw)
To: Michael Witten; +Cc: git
In-Reply-To: <f06dd070abcc485e98c054ec3ee298f9-mfwitten@gmail.com>
Michael Witten <mfwitten@gmail.com> writes:
> According to `git help filter-branch':
>
> --commit-filter <command>
> ...
> You can use the _map_ convenience function in this filter,
> and other convenience functions, too...
> ...
>
> However, it turns out that `map' hasn't been usable because it depends
> on the variable `workdir', which is not propogated to the environment
> of the shell that runs the commit-filter <command>...
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] Documentation/Makefile: add *.pdf to `clean' target
From: Junio C Hamano @ 2011-08-08 17:14 UTC (permalink / raw)
To: Emilio G. Cota; +Cc: git
In-Reply-To: <1312792385-19149-2-git-send-email-cota@braap.org>
Makes sense; thanks.
^ permalink raw reply
* Re: [PATCH 17/18] revert: Introduce --continue to continue the operation
From: Junio C Hamano @ 2011-08-08 16:28 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Christian Couder, Junio C Hamano, Git List, Jonathan Nieder,
Christian Couder, Daniel Barkalow, Jeff King
In-Reply-To: <CALkWK0mKTXUxW=PT_wHEVAzUSgPdZ1s18-9ndZKJqSf6NwVH7w@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Junio: Here's another small fixup patch for your convenience.
Not very convenient I am afraid ;-)
> --8<--
This seems to be too short for "am -c"; have you tried applying it
yourself first before sending it out?
> Subject: [PATCH] fixup! revert: Introduce --continue to continue the
> operation
>
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> t/t3510-cherry-pick-sequence.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 78f3f01..3bca2b3 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -171,7 +171,7 @@ test_expect_success '--continue respects opts' '
> grep "cherry picked from" anotherpick_msg
> '
>
> -test_expect_success '--signoff is not automatically propogated to
> resolved conflict' '
> +test_expect_success '--signoff is not automatically propagated to
> resolved conflict' '
Linewrapped.
I'll squash this manually, so no need to resend. Thanks.
^ permalink raw reply
* Re: [PATCH v2] Tolerate zlib deflation with window size < 32Kb
From: Junio C Hamano @ 2011-08-08 20:07 UTC (permalink / raw)
To: roberto.tyley; +Cc: git, Roberto Tyley
In-Reply-To: <1312742773-26373-1-git-send-email-roberto.tyley@gmail.com>
roberto.tyley@gmail.com writes:
> +test_expect_success setup '
> + cp -R ../t1013/objects .git/
> + git --version
> +'
This "my parent directory" reference ../t1013 does not work when you use a
separate test directory (e.g. to use ramdisks) with --root=/some/where/
I've squashed
cp -R "$TEST_DIRECTORY/t1013/objects" .git/
as a fix while queuing.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] Move git-dir for submodules
From: Heiko Voigt @ 2011-08-08 20:44 UTC (permalink / raw)
To: Fredrik Gustafsson; +Cc: git, jens.lehmann, gitster
In-Reply-To: <1312831022-12868-3-git-send-email-iveqy@iveqy.com>
Hi,
On Mon, Aug 08, 2011 at 09:17:02PM +0200, Fredrik Gustafsson wrote:
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c679f36..1ae6b4e 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -408,6 +408,7 @@ test_expect_success 'submodule update exit immediately in case of merge conflict
> test_cmp expect actual
> )
> '
> +
There is a whitespace error here which we seem to have overlooked. Junio
could you remove that?
> test_expect_success 'submodule update exit immediately after recursive rebase error' '
> (cd super &&
> git checkout master &&
Cheers Heiko
^ permalink raw reply
* Re: Suppressing auto-cc for specific addresses
From: Jeff King @ 2011-08-08 20:44 UTC (permalink / raw)
To: Daniel Mack; +Cc: David Aguilar, Greg KH, git@vger.kernel.org
In-Reply-To: <CACTFLAN0gM-xvEU32KHEsaApH4apvGUwGkiDHx06PngHUvH0Ew@mail.gmail.com>
On Mon, Aug 08, 2011 at 12:58:55PM +0200, Daniel Mack wrote:
> On Mon, Aug 8, 2011 at 12:56 PM, David Aguilar <davvid@gmail.com> wrote:
> > git send-email has a --[no-]signed-off-by-cc option and configuration
> > variable. --
>
> I know, but that's not the point. It about having a certain address
> metioned in a ^Cc: line inside the patch and suppress sending off
> emails to that specific address. And --suppress-cc currently only
> allows categories to be set, not addresses.
I'm kind of confused why you would mark an address as "cc", but not cc
it. Wouldn't it make sense to call the header something else?
-Peff
^ permalink raw reply
* Re: [PATCH] Support specific color for a specific remote branches
From: Jeff King @ 2011-08-08 20:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Aviv Eyal, git
In-Reply-To: <7v8vr3zsh1.fsf@alter.siamese.dyndns.org>
On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:
> > + return 0;
> > +}
> > +
> > +static char *git_branch_get_custom_color_remote(const char *name)
> > +{
> > + int name_len;
> > + char* repo_name;
> > + struct string_list_item *custom;
> > + name_len = strchr(name, '/') - name;
>
> Who said a remote name is terminated with (and cannot contain) a slash?
>
> Shouldn't this code be consulting the configuration file to learn the
> remote mapping, e.g.
>
> [remote "frotz"]
> fetch = +refs/heads/*:refs/remotes/nitfol/*
>
> so that remote branches from "frotz" remote, that happen to be stored
> under refs/remotes/nitfol/ hierarchy, are painted in the correct color?
This seems related to the recent thread about showing branches only for
a specific remote:
http://article.gmane.org/gmane.comp.version-control.git/178668
Maybe the two should share code.
Right now, "git branch -r" means "show everything under refs/remotes
instead of refs/heads". This would be easy to implement if it instead
meant "show all refs created by the RHS of a fetch refspec in a
configured remote". The two are equivalent in the default config, but
the latter may make more sense in a complex case.
-Peff
^ permalink raw reply
* Re: [PATCH 26/48] merge-recursive: Allow make_room_for_path() to remove D/F entries
From: Elijah Newren @ 2011-08-08 20:56 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, jgfouca
In-Reply-To: <4E1D46A5.6050904@viscovery.net>
Hi,
On Wed, Jul 13, 2011 at 1:17 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 6/8/2011 9:30, schrieb Elijah Newren:
>> +static int make_room_for_path(const struct merge_options *o, const char *path)
>> {
>> - int status;
>> + int status, i;
>> const char *msg = "failed to create path '%s'%s";
>>
>> + /* Unlink any D/F conflict files that are in the way */
>> + for (i = 0; i < o->df_conflict_file_set.nr; i++) {
>> + const char *df_path = o->df_conflict_file_set.items[i].string;
>> + size_t pathlen = strlen(path);
>> + size_t df_pathlen = strlen(df_path);
>> + if (df_pathlen < pathlen && strncmp(path, df_path, df_pathlen) == 0) {
>> + unlink(df_path);
>> + break;
>> + }
>> + }
>
> Each time this loop is entered it tries to remove the same path again,
> even if it does not exist anymore or was morphed into a directory in the
> meantime. I suggest to remove a path from o->df_conflict_file_set after it
> was unlinked. Or even better: have a separate "make room" phase somewhere
> in the merge process.
Removing it from o->df_conflict_file_set makes sense. However, there
appears to be no API in string_list.h for deleting entries. Are such
operations discouraged? I'm not sure whether to add such API, just
hack it directly, or wait for someone else to come along and change
this to a better data structure (such as a hash)...
I don't think it's possible to move this "make room" phase anywhere
earlier in the merge process. When we have D/F conflicts, the files
of those D/F conflicts should only be removed if at least one of the
paths under the corresponding directory are not removed by the merge
process. We don't know whether those paths will need to be removed
until we call process_entry() on each of them, and from there we go
right to this function when we find one that needs to stick around.
So I simply don't see how to move it any earlier.
However, there is clearly a bug in this code. If there is a D/F
conflict at 'd' (e.g. paths 'd' and 'd/foo' are present), and there is
a file named 'd_bla', then the need to merge 'd_bla' can cause 'd' to
be deleted. Oops. (Granted, it's a bug that would be masked by the
later call to process_entry() on 'd', which would reinstate that file
if necessary, but that doesn't mean this code is right.) I'll fix
that up.
However, I don't see how any of this would address any failure you're
seeing on windows. Maybe one of my other changes, including one or
two other bugfixes I've found will help? I'll have to ping you when I
submit the re-roll.
^ permalink raw reply
* Re: Suppressing auto-cc for specific addresses
From: Daniel Mack @ 2011-08-08 21:01 UTC (permalink / raw)
To: Jeff King; +Cc: David Aguilar, Greg KH, git@vger.kernel.org
In-Reply-To: <20110808204448.GF18294@sigill.intra.peff.net>
On Mon, Aug 8, 2011 at 10:44 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 08, 2011 at 12:58:55PM +0200, Daniel Mack wrote:
>
>> On Mon, Aug 8, 2011 at 12:56 PM, David Aguilar <davvid@gmail.com> wrote:
>> > git send-email has a --[no-]signed-off-by-cc option and configuration
>> > variable. --
>>
>> I know, but that's not the point. It about having a certain address
>> metioned in a ^Cc: line inside the patch and suppress sending off
>> emails to that specific address. And --suppress-cc currently only
>> allows categories to be set, not addresses.
>
> I'm kind of confused why you would mark an address as "cc", but not cc
> it. Wouldn't it make sense to call the header something else?
Valid point. Even though Greg doesn't have a problem with the current
procedure as it stands, I see a problem in marking things for "Cc:
stable@kernel.org" just as a search pattern once they hit the Linux
mainline tree. Particularily because as it will end up in the
recipient list with git send-email, stable@ will get all the
follow-ups and discussions when people blindly hit the Reply-to-all
button (which we tell them to do). I for one would add
stable@kernel.org to my .gitconfig and be done with it, without ever
thinking about it again.
It's really not a big thing, and we can drop this patch if anyone has
real objections, but as it doesn't break anything for existing use
cases and might help others, why not apply it?
Daniel
^ permalink raw reply
* Re: [RFC] helping smart-http/stateless-rpc fetch race
From: Sverre Rabbelier @ 2011-08-08 21:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn Pearce, git
In-Reply-To: <7vsjpbzv07.fsf@alter.siamese.dyndns.org>
Heya,
On Mon, Aug 8, 2011 at 19:13, Junio C Hamano <gitster@pobox.com> wrote:
>> (1) It might make sense to give admins who run upload-pack not behind
>> smart-http an option to allow fetching from a non-tip; and
You said earlier it isn't needed since the server process caches the
refs for git and ssh, that leaves dumb-http right? If that is indeed
the case I think we can just argue that since smart-http is our
solution to the http problems, if admins want to make life easier for
http fetches on busy repositories they should be using smart-http.
>> (2) It also might make sense to let admins who do run upload-pack behind
>> smart-http force re-fetching when the race is encountered.
This would mean that if you're running smart-http without this option
enabled (because, say, you don't know it exists), your users have to
re-fetch (a lot). The only upside would be that if the server _knows_
what the user is asking for is outdated, that the user will know this
right away. That doesn't fly though, since we allow exactly that for
git and ssh transfer.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: Suppressing auto-cc for specific addresses
From: Jonathan Nieder @ 2011-08-08 21:07 UTC (permalink / raw)
To: Jeff King; +Cc: Daniel Mack, David Aguilar, Greg KH, git
In-Reply-To: <20110808204448.GF18294@sigill.intra.peff.net>
Jeff King wrote:
> I'm kind of confused why you would mark an address as "cc", but not cc
> it. Wouldn't it make sense to call the header something else?
Maybe the patch is a draft and he is seeking early feedback. The
final version will be cc-ed to the indicated person later (and that's
part of what it is time to get feedback on).
Or maybe the Cc: line is from the original patch and he is using git
send-email to forward it without mangling.
^ permalink raw reply
* Re: Suppressing auto-cc for specific addresses
From: Greg KH @ 2011-08-08 21:11 UTC (permalink / raw)
To: Daniel Mack; +Cc: Jeff King, David Aguilar, git@vger.kernel.org
In-Reply-To: <CACTFLAN1oXHtEtoo7kzvTqaiSv0zvmbkPU703iZsztCWdg4HhQ@mail.gmail.com>
On Mon, Aug 08, 2011 at 11:01:12PM +0200, Daniel Mack wrote:
> On Mon, Aug 8, 2011 at 10:44 PM, Jeff King <peff@peff.net> wrote:
> > On Mon, Aug 08, 2011 at 12:58:55PM +0200, Daniel Mack wrote:
> >
> >> On Mon, Aug 8, 2011 at 12:56 PM, David Aguilar <davvid@gmail.com> wrote:
> >> > git send-email has a --[no-]signed-off-by-cc option and configuration
> >> > variable. --
> >>
> >> I know, but that's not the point. It about having a certain address
> >> metioned in a ^Cc: line inside the patch and suppress sending off
> >> emails to that specific address. And --suppress-cc currently only
> >> allows categories to be set, not addresses.
> >
> > I'm kind of confused why you would mark an address as "cc", but not cc
> > it. Wouldn't it make sense to call the header something else?
>
> Valid point. Even though Greg doesn't have a problem with the current
> procedure as it stands, I see a problem in marking things for "Cc:
> stable@kernel.org" just as a search pattern once they hit the Linux
> mainline tree. Particularily because as it will end up in the
> recipient list with git send-email, stable@ will get all the
> follow-ups and discussions when people blindly hit the Reply-to-all
> button (which we tell them to do). I for one would add
> stable@kernel.org to my .gitconfig and be done with it, without ever
> thinking about it again.
No, that's a good thing, as the stable tree maintainer, I _want_ to see
that discussion to verify that this patch really is something that
should be applied to a stable kernel tree.
Lots of patches have originally been tagged in this manner, added to the
git tree, yet in discussion threads afterward, it's been determined that
this should not go to the stable tree. If I hadn't been on that thread
in the first place, I would have missed that.
So again, I have no objection to cc: stable@kernel.org, and in fact,
encourage it. Don't worry, I can handle the email load :)
thanks,
greg k-h
^ permalink raw reply
* rebase -i: Should --continue auto-amend after failed exec?
From: Johannes Sixt @ 2011-08-08 21:11 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git@vger.kernel.org List
If after a failed "exec" instruction there are staged changes, then currently rebase
--continue fails with:
.../git-rebase--interactive: line 774: .../.git/rebase-merge/author-script: No such file or directory
But shouldn't this amend the HEAD commit? The documentation is not clear
(from git-rebase.txt):
The interactive rebase will stop when a command fails (i.e. exits with
non-0 status) to give you an opportunity to fix the problem. You can
continue with `git rebase --continue`.
This may be interpreted to work like "edit", and IMO would be a very useful
modus operandi.
Here is a test case.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 47c8371..2146e47 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -527,6 +527,21 @@ test_expect_success 'auto-amend only edited commits after "edit"' '
git rebase --abort
'
+test_expect_failure 'auto-amend after failed "exec"' '
+ test_tick &&
+ test_when_finished "git rebase --abort || :" &&
+ (
+ FAKE_LINES="1 exec_false" &&
+ export FAKE_LINES &&
+ test_must_fail git rebase -i HEAD^
+ ) &&
+ echo "edited again" > file7 &&
+ git add file7 &&
+ FAKE_COMMIT_MESSAGE="edited file7 again" git rebase --continue &&
+ actual=$(git show HEAD:file7) &&
+ test "edited again" = "$actual"
+'
+
test_expect_success 'rebase a detached HEAD' '
grandparent=$(git rev-parse HEAD~2) &&
git checkout $(git rev-parse HEAD) &&
^ permalink raw reply related
* Re: Suppressing auto-cc for specific addresses
From: Jonathan Nieder @ 2011-08-08 21:17 UTC (permalink / raw)
To: Daniel Mack
Cc: git, Greg Kroah-Hartman, Joe Perches,
Ævar Arnfjörð Bjarmason, J. Bruce Fields,
Jeff King
In-Reply-To: <4E3EF38A.9010307@gmail.com>
(+cc: some send-email people)
Hi,
Daniel Mack wrote:
> I'm looking for a way to suppress certain Cc: fields that are normally
> automatically added for addresses mentioned in Cc: lines found in patches.
Makes sense. Could you please add a quick commit message, following
the guidelines in Documentation/SubmittingPatches (i.e., explaining
the problem this intends to solve and justifying how it is solved)?
Patch left untrimmed for Joe and Ævar's convenience. Thanks and hope
that helps.
> From: Daniel Mack <zonque@gmail.com>
> Date: Sun, 7 Aug 2011 22:16:00 +0200
> Subject: [PATCH] send-email: Allow email addresses and domains as
> --suppress-cc options
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
> Documentation/git-send-email.txt | 10 +++++++---
> git-send-email.perl | 12 +++++++++++-
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 327233c..5966abb 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -233,9 +233,9 @@ Automating
> cc list. Default is the value of 'sendemail.signedoffbycc' configuration
> value; if that is unspecified, default to --signed-off-by-cc.
>
> ---suppress-cc=<category>::
> - Specify an additional category of recipients to suppress the
> - auto-cc of:
> +--suppress-cc=<category|address|domain>::
> + Specify an additional category, email address or domain of recipients
> + to suppress the auto-cc for. Possible categories are:
> +
> --
> - 'author' will avoid including the patch author
> @@ -251,6 +251,10 @@ Automating
> - 'all' will suppress all auto cc values.
> --
> +
> +If a full email address is given, auto-cc will be suppressed for this particular
> +address. This also works for entire domains if the parameter starts with the '@'
> +sign. All email addresses in that domain will hence be ignored by auto-cc.
> ++
> Default is the value of 'sendemail.suppresscc' configuration value; if
> that is unspecified, default to 'self' if --suppress-from is
> specified, as well as 'body' if --no-signed-off-cc is specified.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 98ab33a..22813e5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -374,7 +374,7 @@ my(%suppress_cc);
> if (@suppress_cc) {
> foreach my $entry (@suppress_cc) {
> die "Unknown --suppress-cc field: '$entry'\n"
> - unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc)$/;
> + unless $entry =~ /^(?:all|cccmd|cc|author|self|sob|body|bodycc|.*@.*)$/;
> $suppress_cc{$entry} = 1;
> }
> }
> @@ -1213,6 +1213,11 @@ foreach my $t (@files) {
> } else {
> next if ($suppress_cc{'cc'});
> }
> + next if ($suppress_cc{$addr});
> + if ($addr =~ /^(.+)(@.+)$/) {
> + my $domain = $2;
> + next if $suppress_cc{$domain};
> + }
> printf("(mbox) Adding cc: %s from line '%s'\n",
> $addr, $_) unless $quiet;
> push @cc, $addr;
> @@ -1261,6 +1266,11 @@ foreach my $t (@files) {
> next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
> next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
> }
> + next if ($suppress_cc{$c});
> + if ($c =~ /^(.+)(@.+)$/) {
> + my $domain = $2;
> + next if $suppress_cc{$domain};
> + }
> push @cc, $c;
> printf("(body) Adding cc: %s from line '%s'\n",
> $c, $_) unless $quiet;
> --
> 1.7.6
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Sverre Rabbelier @ 2011-08-08 21:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Jonathan Nieder, Jeff King, Git List,
Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <7vfwlbztfg.fsf@alter.siamese.dyndns.org>
Heya,
On Mon, Aug 8, 2011 at 19:47, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> If you do that, you're back to start. Since obj has not the faintest clue
>> whether the pending object was added from a negative or a positive ref.
>
> But the point is that this codepath does not have a faintest clue whether
> the "obj" parameter is something the end user actively asked for (which
> might have been marked as uninteresting for other reasons, namely, because
> it is reachable from other negative refs). So passing unconditional 0 is
> just as bad.
Doesn't passing 0 indicate that we _did not_ receive any explicit user
input on this ref, which is exactly what we want to record? If the
user passed us any explicit input on the ref, we record it at the
other call-sites, here, the user told us nothing, so we record exactly
that, nothing.
>> Is this not a little bit of a big, huge, tremendous overkill?
>
> As long as you can show your "flags" can (be extended to) express the same
> richness to solve sample problems I mentioned in my response, as well as
> your immediate issue, I wouldn't insist implementing a parsed struct/union
> that may be a more (and unnecessarily) verbose way to say the same thing.
I cannot recall you ever asking somebody to implement some feature
_nobody needs right now_ while trying to fix a _bug_, why now? I do
not know this code well enough to implement it, and Dscho doesn't have
the time to do it.
>> Or in other words: I'd rather stay with a simple, elegant, minimal patch
>> that solves the problem at hand while not preventing future enhancements.
>
> We are on the same page, but what I read from the patch didn't show a
> clear way forward to extend the "flags" to allow the stuff I mentioned
Nobody needs the stuff you mentioned right now. We do need this to fix
this bug. If someone else does want it at a later date, replacing it
will be exactly as much work with or without this patch.
> I would be reluctant to accept a myopic hack that is only good for one
> caller and that needs to be ripped out and re-done, especially when we
> already know other issues that can be solved cleanly if you go a little
> further in the initial round.
While I understand this reluctance, remember that this "one caller" is
required to fix a bug in the current code. If you had a similar
complaint about the remote-hg.py patches that I haven't sent yet, I
would be more than willing to invest the extra time in addressing
those concerns, since I'm adding new functionality anyway, this is
different.
> As I said, I am not married to the verbose struct/union representation
> (the only reason I showed that verbosity was because it allowed me to do
> away without having to enumerate all the syntax sugars we already
> support); if your "flags" can express the same thing (it may needs to
> become a bitfield with enough width, but I highly suspect that you would
> also need at least a component that says "this is the string the user gave
> us --- the user said 'master', not 'master^0', for example) and is a lot
> more compact, that is definitely we want to go with.
Don't we already store that in the name field?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Support specific color for a specific remote branches
From: Junio C Hamano @ 2011-08-08 21:31 UTC (permalink / raw)
To: Jeff King; +Cc: Aviv Eyal, git
In-Reply-To: <20110808205214.GG18294@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Aug 08, 2011 at 11:08:26AM -0700, Junio C Hamano wrote:
>
> This seems related to the recent thread about showing branches only for
> a specific remote:
>
> http://article.gmane.org/gmane.comp.version-control.git/178668
Yeah, that one I do recall.
> Right now, "git branch -r" means "show everything under refs/remotes
> instead of refs/heads". This would be easy to implement if it instead
> meant "show all refs created by the RHS of a fetch refspec in a
> configured remote". The two are equivalent in the default config, but
> the latter may make more sense in a complex case.
I actually am a bit ambivalent about this. I do not necessarily consider
the contrived "remote.frotz.fetch = refs/heads/*:refs/remotes/nitfol/*"
example something that we _must_ solve. It is unlikely people would do
that, and if we give them an unexpected result, they deserve it ;-).
But in real-life, it is entirely plausible that people with multiple
integration branches are taking advantage of the simplicity of the old
layout, i.e.
[remote "origin"]
fetch = refs/heads/master:refs/heads/origin
fetch = refs/heads/next:refs/heads/next
fetch = refs/heads/maint:refs/heads/maint
fetch = +refs/heads/pu:refs/heads/pu
I have many repositories of this style, and it is very convenient to be
able to say:
$ git checkout master && git pull --ff-only
$ for b in master next maint pu
do
git checkout $b && make install || break
done
I do not think I want to ever switch them to new layout, and I suspect
that many others do feel the same.
Now, for these repositories, is "next" a local branch or a remote one? I
have a feeling that it might be easier to understand if we label anything
that you can update with "checkout && commit" a local one for the purpose
of "branch -r" listing; IOW, the current "git branch -r" classification
would match this use pattern better, even though refs/heads/next _is_ an
RHS of a rule to follow others.
In that sense, I would be entirely happy if the configuration variable
used in this series were branch.<namepattern>.color and let you specify
[branch "refs/heads"] color = yellow
[branch "refs/remotes/origin"] color = purple
[branch "refs/remotes/nitfol"] color = cyan
It becomes complicated (and for no good reason, in my opinion; see the
"next" example above) if you try to tie this with remote.<name> hierarchy,
as it obviously becomes illogical not to use the "RHS of a fetch refspec"
logic when we are talking about remote.<name>.
^ permalink raw reply
* Re: [PATCH v2 2/2] Move git-dir for submodules
From: Junio C Hamano @ 2011-08-08 21:34 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Fredrik Gustafsson, git, jens.lehmann, gitster
In-Reply-To: <20110808204439.GA41500@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> Hi,
>
> On Mon, Aug 08, 2011 at 09:17:02PM +0200, Fredrik Gustafsson wrote:
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index c679f36..1ae6b4e 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -408,6 +408,7 @@ test_expect_success 'submodule update exit immediately in case of merge conflict
>> test_cmp expect actual
>> )
>> '
>> +
>
> There is a whitespace error here which we seem to have overlooked. Junio
> could you remove that?
Hmm, confused.
A blank between test would be the perfectly normal thing to see, no?
>> test_expect_success 'submodule update exit immediately after recursive rebase error' '
>> (cd super &&
>> git checkout master &&
>
> Cheers Heiko
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Junio C Hamano @ 2011-08-08 22:07 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Johannes Schindelin, Jonathan Nieder, Jeff King, Git List,
Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Nobody needs the stuff you mentioned right now.
Wrong.
"diff ^C A B" is a bug, isn't it? And that is a bug in the current code.
>> I would be reluctant to accept a myopic hack that is only good for one
>> caller and that needs to be ripped out and re-done, especially when we
>> already know other issues that can be solved cleanly if you go a little
>> further in the initial round.
>
> While I understand this reluctance, remember that this "one caller" is
> required to fix a bug in the current code.
^ permalink raw reply
* Re: [PATCH 37/48] merge-recursive: Fix modify/delete resolution in the recursive case
From: Elijah Newren @ 2011-08-08 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jgfouca
In-Reply-To: <7voc0n1nl8.fsf@alter.siamese.dyndns.org>
On Thu, Jul 21, 2011 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> When o->call_depth>0 and we have conflicts, we try to find "middle ground"
>> when creating the virtual merge base. In the case of content conflicts,
>> this can be done by doing a three-way content merge and using the result.
>> In all parts where the three-way content merge is clean, it is the correct
>> middle ground, and in parts where it conflicts there is no middle ground
>> but the conflict markers provide a good compromise since they are unlikely
>> to accidentally match any further changes.
>>
>> In the case of a modify/delete conflict, we cannot do the same thing.
>> Accepting either endpoint as the resolution for the virtual merge base
>> runs the risk that when handling the non-recursive case we will silently
>> accept one person's resolution over another without flagging a conflict.
>> In this case, the closest "middle ground" we have is actually the merge
>> base of the candidate merge bases. (We could alternatively attempt a
>> three way content merge using an empty file in place of the deleted file,
>> but that seems to be more work than necessary.)
>
> What did we use before this patch as the "middle ground"?
We used precisely what you seem to suggest at the end of your email.
It's a trade-off, and I agree with you about the cases where my new
choice is suboptimal, but I personally think the original choice has
worse trade-offs.
> Doesn't the "middle ground" also need to contain information from both
> sides? At the content level, The half-merge result with conflict marker
> contains all the necessary information on what both sides agreed to do
> outside the marker area, while showing what each side wished to do inside,
> so as you describe, it is a good "middle ground".
>
> Using the merge-base would mean that at the tree-structural level of this
> merge you ignore the wish of one side that wanted to delete the path, and
> then at the content level you also ignore the wish of both sides (the side
> that wanted to delete the path wanted to leave zero content, while the
> other side wanted to modify from the version in merge-base, all of which
> is ignored by the above argument).
I think it is optimal if the "middle ground" can contain information
from both sides, but I simply do not see how it is possible in either
a modify/delete or rename/delete case. That is, at least not without
some kind of significant change to the index data structures or
allowing a "virtual merge base" to be an index with higher stage merge
entries instead of just a tree (though that would introduce several
new complications).
And yes, I am essentially ignoring the intents of both sides, and
intentionally so. I thought I addressed that in the arguments above,
but perhaps I could have been more clear and explicit about that.
Your explanation below provides a great framework for clarifying why I
made this change, so let me respond below using the framework you have
beautifully illustrated and explained.
> When you want to make a criss-corss merge between E and F that looks like
> this:
>
> A---C---E---?
> / \ / /
> O . /
> \ / \ /
> B---D---F
>
> if there is no deletion, we run a content level merge between C and D by
> using X that is a potentially conflicting merge between A and B as a
> "virtual" ancestor.
>
> A---C---E---?
> / \ / /
> O X /
> \ / \ /
> B---D---F
>
> X would contain both what both A and B agreed to do on top of O, and what
> A and B wanted to do that they do not agree with. The differing opinion of
> A and B are recorded inside the conflict markers. The change that turns X
> into E contains C's opinion (i.e. it would likely agree to take what both
> A and B agreed to do, and it may agree with A and the result in E would
> resemble what A wanted to bring to the result despite B's objection). If C
> and D both agree how to resolve the conflict, then B's "objection" will
> cancel out from the three-way merge between E and F that uses X as the
> ancestor.
>
> In a delete/modify situation (e.g. A modified while B deleted), there are
> three possibilities:
>
> * C and D both agree to delete the path;
>
> * C and D both agree to keep the path, with modifications that may or may
> not conflict (which can be handled by the usual content-level merge);
>
> * C decides to delete, while D decides to modify (or vice versa).
This is a really nicely written explanation. I particularly like this
list of three cases. Let me augment this list of situations (which
I'll number (1)-(3)) with a list of four choices for virtual merge
base X:
(J) Make X not have any content at the path (i.e. accept the deletion in B)
(K) Make X have the content from O at path (i.e. what my patch does)
(L) Make X have the content from A at path (which we did before)
(M) Make X have some other content at path
I'll get back to these 12 situations (3 cases each with how any of 4
choices could affect them) in a minute...
> And in the last case, the outer merge ? needs to decide if it wants to
> keep or delete the path anyway, so a simplest solution is to punt the
> whole recursive business and make it the responsibility of the user to
> resolve it as a merge between E and F using O as the common ancestor.
> This patch does so in all three cases.
>
> I however wonder if we can do better in the second case (I do not think
> the first case would come into the picture, as we would not see such a
> path when merging E and F as it would have been long gone from both
> branches). We wouldn't know which commits C and D are exactly, but we do
> have E and F. If A modified the path and B deleted it, and C and D both
> decided to keep the path and E and F both inherited that path, wouldn't it
> be fair to say that what both branches wanted to do is closer to what A
> did than what B did? In other words, instead of using O, wouldn't it give
> us a better result if we used A (the side that did not delete) as the
> common ancestor for the content level merge when both E and F has the
> path?
Yes, absolutely. In the second case, using the content of the path
from A for the virtual merge ancestor would be a slight improvement
over using the content for path from O. But let's look at all three
cases you specified and how different choices for the virtual merge
base affect it. Here are my goals for any merge, in priority order:
1st goal: detect real conflicts and notify the user
2nd goal: avoid conflicts in cases where we should be able to
reasonably confidently merge cleanly
3rd goal: make it easy for the user to figure out how to resolve problems
Obviously, we can't always meet these goals, particularly due to
"semantic" conflicts that may be present. But they're useful to keep
in mind. Here is how I broke down the three cases:
Case 1: C and D both agree to delete the path
In this case, regardless of the choice (J)-(M), the merge will be
resolved correctly.
Case 2: C and D both agree to keep the path, with modifications that
may or may not conflict (which can be handled by the usual
content-level merge);
In this case, choice (J) would cause content conflicts whenever C & D
had different content at path and would treat it as a two-way content
merge. That makes choice (J) particularly problematic as it breaks
the first goal. In order to get the merge right, we really need to do
a three-way content merge, using content that can serve as a
reasonable merge-base for the contents chosen at C & D. That makes
choice (M) for X a poor one. Both choices (K) and (L) are reasonable,
with (L) being a better choice (as you also point out above) to
optimize the 2nd goal.
Case 3: C decides to delete, while D decides to modify (or vice versa)
It is worth noting here that the most likely choice for the content at
path for D is exactly the content that is at A. git is designed for
trees of slowly changing data after all.
This is a case where git should detect the modify/delete conflict and
alert the user. In this case, choice (J) is particularly bad.
Regardless of the content that D has for path, the merge conflict
won't even be detected and git would silently resolve this by
recording the content from D at path. That's a fail for the first
goal.
Another choice that is likely to fail the first goal is (L). If the
content at path D is the same as at A (the most likely case), then git
would not detect the conflict and would simply silently delete the
path.
In fact, any choice here for the merge base could cause the conflict
to go silently undetected (whenever the content at D matched our
choice for X), meaning that no choice for the merge base will be
perfect. That would mean of the four choices, (M) would be best as it
would be least likely to match the choice made at D, but (K) would be
pretty reasonable.
So, as I view it: choice (J) is horrible in two of the three cases;
choice (M) is really bad in the second case for goal 2; and choice (L)
is likely to be bad in the third case for goal 1. Choice (K) is
pretty reasonable in all three cases, and that choice is what this
patch implements.
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Junio C Hamano @ 2011-08-08 22:24 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Johannes Schindelin, Jonathan Nieder, Jeff King, Git List,
Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <CAGdFq_hLy6_AW-Yh_9fi318Z6jdkFWw5+cYrwMtOitDkGQorFA@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
>> But the point is that this codepath does not have a faintest clue whether
>> the "obj" parameter is something the end user actively asked for (which
>> might have been marked as uninteresting for other reasons, namely, because
>> it is reachable from other negative refs). So passing unconditional 0 is
>> just as bad.
>
> Doesn't passing 0 indicate that we _did not_ receive any explicit user
> input on this ref, which is exactly what we want to record? If the
> user passed us any explicit input on the ref, we record it at the
> other call-sites, here, the user told us nothing, so we record exactly
> that, nothing.
Hmm, which means you have a way to say "explicitly affirmative" vs "no
information", but no way to say "explicitly negative", for example, and
the worse part is that it is unclear if the approach the patch takes is
extensible enough to allow that in the future. That is the kind of "myopic
hack" attitude I did not particularly like in this patch.
"The next person who needs more generic framework can rip out what this
patch does and the work required is the same amount" is not a convincing
argument---it would mean you are burdening that other person with an extra
work to _redo_ what this series does properly, and it is not likely to be
of help for that person after your interest in this codepath has long
waned.
>> As I said, I am not married to the verbose struct/union representation
>> (the only reason I showed that verbosity was because it allowed me to do
>> away without having to enumerate all the syntax sugars we already
>> support); if your "flags" can express the same thing (it may needs to
>> become a bitfield with enough width, but I highly suspect that you would
>> also need at least a component that says "this is the string the user gave
>> us --- the user said 'master', not 'master^0', for example) and is a lot
>> more compact, that is definitely we want to go with.
>
> Don't we already store that in the name field?
Please remind yourself why then it is not sufficient for your patch to
read from the name field please?
After all, wasn't the issue that "master^0..master" yields an empty set
but you somehow wanted to know that the RHS of that dotdot was given as a
positive ref?
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Sverre Rabbelier @ 2011-08-08 22:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Jonathan Nieder, Jeff King, Git List,
Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <7vpqkfv8x1.fsf@alter.siamese.dyndns.org>
Heya,
On Tue, Aug 9, 2011 at 00:24, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, which means you have a way to say "explicitly affirmative" vs "no
> information", but no way to say "explicitly negative", for example, and
> the worse part is that it is unclear if the approach the patch takes is
> extensible enough to allow that in the future. That is the kind of "myopic
> hack" attitude I did not particularly like in this patch.
Do we have a way to say explicitly negative on the commandline? Is
there a way to say "I don't want you to decorate this commit as
anything at all"?
> "The next person who needs more generic framework can rip out what this
> patch does and the work required is the same amount" is not a convincing
> argument---it would mean you are burdening that other person with an extra
> work to _redo_ what this series does properly, and it is not likely to be
> of help for that person after your interest in this codepath has long
> waned.
Be fair. How is "2 files changed, 8 insertions(+), 6 deletions(-)"
going to make it any harder at all for someone who is going to be
doing that huge patch series you described earlier?
>> Don't we already store that in the name field?
>
> Please remind yourself why then it is not sufficient for your patch to
> read from the name field please?
Sure, we could do it. But it would be duplicating all the effort
already being done in rev-parse!
> After all, wasn't the issue that "master^0..master" yields an empty set
> but you somehow wanted to know that the RHS of that dotdot was given as a
> positive ref?
The issue was that if I push "master" to origin but I already pushed a
"next" which points at the same commit nothing happens because
git-fast-export doesn't know to emit the "reset master :13878" line.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Sverre Rabbelier @ 2011-08-08 22:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Jonathan Nieder, Jeff King, Git List,
Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <7vty9rv9p2.fsf@alter.siamese.dyndns.org>
Heya,
On Tue, Aug 9, 2011 at 00:07, Junio C Hamano <gitster@pobox.com> wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
>> Nobody needs the stuff you mentioned right now.
>
> Wrong.
>
> "diff ^C A B" is a bug, isn't it? And that is a bug in the current code.
I think I missed something, can you point me to the relevant thread
about this bug in diff? Maybe I can convince the people involved in
fixing that to help out.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not
From: Junio C Hamano @ 2011-08-08 22:36 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, Johannes Schindelin, Jonathan Nieder, Jeff King,
Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov
In-Reply-To: <CAGdFq_joHskwhp=934OjirmXiRMR3NbGd4s-hSjJc-gCFT_Jew@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Tue, Aug 9, 2011 at 00:07, Junio C Hamano <gitster@pobox.com> wrote:
>> Sverre Rabbelier <srabbelier@gmail.com> writes:
>>> Nobody needs the stuff you mentioned right now.
>>
>> Wrong.
>>
>> "diff ^C A B" is a bug, isn't it? And that is a bug in the current code.
>
> I think I missed something, can you point me to the relevant thread
> about this bug in diff? Maybe I can convince the people involved in
> fixing that to help out.
It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>
^ 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