* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jonathan Nieder @ 2012-10-31 0:37 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <1351623987-21012-5-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras wrote:
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
> typename(e->item->type));
> continue;
> }
> - if (commit->util) {
> - /* more than one name for the same object */
> +
> + /*
> + * This ref will not be updated through a commit, lets make
> + * sure it gets properly upddated eventually.
> + */
> + if (commit->util || commit->object.flags & SHOWN) {
> if (!(commit->object.flags & UNINTERESTING))
> string_list_append(extra_refs, full_name)->util = commit;
> - } else
> + }
> + if (!commit->util)
> commit->util = full_name;
Here's an explanation of why the above makes sense to me.
get_tags_and_duplicates() gets called after the marks import and
before the revision walk. It walks through the revs from the
commandline and for each one:
- peels it to a refname, and then to a commit
- stores the refname so fast-export knows what arg to pass to
the "commit" command during the revision walk
- if it already had a refname stored, instead adds the
(refname, commit) pair to the extra_refs list, so fast-export
knows to add a "reset" command later.
If the commit already has the SHOWN flag set because it was pointed to
by a mark, it is not going to come up in the revision walk, so it will
not be mentioned in the output stream unless it is added to
extra_refs. That's what this patch does.
Incidentally, the change from "else" to "if (!commit->util)" is
unnecessary because if a commit is already SHOWN then it will not be
encountered in the revision walk so commit->util does not need to be
set.
If the commit does not have the SHOWN or UNINTERESTING flag set but it
is going to get the UNINTERESTING flag set during the walk because of
a negative commit listed on the command line, this patch won't help.
Jonathan
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-31 0:57 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121030185914.GI15167@elie.Belkin>
Hi again,
Felipe Contreras wrote:
> They have been marked as UNINTERESTING for a reason, lets respect that.
So, the above description conveyed zero information, as you mentioned.
A clearer explanation would be the following:
fast-export: don't emit "reset" command for negative refs
When "git fast-export" encounters two refs on the commandline
referring to the same commit, it exports the first during the usual
commit walk and the second using a "reset" command in a final pass
over extra_refs:
$ git fast-export master next
reset refs/heads/master
commit refs/heads/master
mark :1
author Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
committer Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
data 17
My first commit!
reset refs/heads/next
from :1
Unfortunately the code to do this doesn't distinguish between positive
and negative refs, producing confusing results:
$ git fast-export ^master next
reset refs/heads/next
from :0
$ git fast-export master ^next
reset refs/heads/next
from :0
Use revs->cmdline instead of revs->pending to iterate over the rev-list
arguments, checking the UNINTERESTING flag bit to distinguish between
positive (master, --all, etc) and negative (next.., --not --all, etc)
revs and avoid enqueueing negative revs in extra_revs.
This does not affect revs that were excluded from the revision walk
because pointed to by a mark, since those use the SHOWN bit on the
commit object itself and not UNINTERESTING on the rev_cmdline_entry.
A patch meeting the above description would make perfect sense to me.
Except for the somewhat strange testcase, the patch I am replying to
would also be fine in the short term, as long as it had an analagous
description (i.e., with an appropriate replacement for the
second-to-last paragraph).
Thanks for your patience, and hoping that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-31 1:03 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121030235506.GT15167@elie.Belkin>
On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Nope. I just don't want regressions, and found a patch description
>>> that did nothing to explain to the reader how it avoids regressions
>>> more than a little disturbing.
>>
>> I see, so you don't have any specific case where this could cause
>> regressions, you are just saying it _might_ (like all patches).
>
> Yes, exactly. The commit log needs a description of the current
> behavior, the intent behind the current code, the change the patch
> makes, and the motivation behind that change, like all patches.
> Despite the nice examples, it doesn't currently have that.
>
> The patch description just raises more questions for the reader. From
> the description, one might imagine that this patch causes
>
> git fast-export <mark args> master
>
> not to emit anything when another branch that has already been
> exported is ahead of "master".
This is already the case.
I don't see what part of my patch description would give you the idea
that this would change in any way how the objects are flagged, or how
get_revision() decides how to traverse them.
I clearly stated that this doesn't affect *the first* ref, which is
handled properly already; this patch affects *the rest* of the refs,
of which you have none in that command above.
> If I understand correctly (though
> I haven't tested), this patch does cause
>
> git fast-export ^next master
>
> not to emit anything when next is ahead of "master". That doesn't
> seem like progress.
Again, this is already the case RIGHT NOW.
And nothing in my description should give you an idea that anything
would change for this case because the 2nd ref (*the first* doesn't
get affected), is not marked as UNINTERESTING.
Not only you are not reading what is in the description, but I don't
think you understand what the code actually does, and how it behaves.
Let me give you some examples:
% git fast-export ^next next
reset refs/heads/next
from :0
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
The *only time* when this patch would have any effect is when you
specify more than *one ref*, and they both point to *exactly the same
object*.
Additionally, and this is something I just found out; when the are
pure refs (e.g. 'next'), and not refs to objects (e.g.
'next^{commit}').
In any other case; *there would be no change*.
After my patch:
% git fast-export ^next next
# nothing
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
> But in the long term it is much easier to understand
> and maintain a patch series that does not introduce regressions in the
> first place
It does not introduce regressions.
I don't think it's my job to explain to you how 'git fast-export'
works. Above you made too many assumptions of what get broken, when in
fact that's the current behavior already... maybe, just maybe, you are
also making wrong assumptions about this patch as well.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-31 1:08 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s1ftDijYpZW_Reu5qNi1T_L52_353ngNaRW3W1gz+k9jw@mail.gmail.com>
Felipe Contreras wrote:
> I don't think it's my job to explain to you how 'git fast-export'
> works.
Actually, if you are submitting a patch for inclusion, it is your job
to explain to future readers what the patch does. Yes, the reader
might not be deeply familiar with the part of fast-export you are
modifying. It might have even been modified since then, by the time
the reader is looking at the change!
Sad but true.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-31 1:23 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031005748.GW15167@elie.Belkin>
Hi,
On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> So, the above description conveyed zero information, as you mentioned.
I meant, this, of course:
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> This patch looks unsafe,
Which you know, because you received that message without the mistake.
> A clearer explanation would be the following:
>
> fast-export: don't emit "reset" command for negative refs
What is a negative ref?
> When "git fast-export" encounters two refs on the commandline
commandline?
Only two refs? How about four?
> referring to the same commit, it exports the first during the usual
> commit walk and the second using a "reset" command in a final pass
> over extra_refs:
That is not exactly true: (next^{commit}).
> $ git fast-export master next
> reset refs/heads/master
> commit refs/heads/master
> mark :1
> author Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
> committer Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
> data 17
> My first commit!
>
> reset refs/heads/next
> from :1
I don't think this example is good. Where does it say that 'next'
points to master? Using 'points-to-master' or a 'git branch stable
master' and using 'master stable'.
Even simpler would be to use 'git fast-export master master'; it would
show the same behavior.
> Unfortunately the code to do this doesn't distinguish between positive
> and negative refs, producing confusing results:
>
> $ git fast-export ^master next
> reset refs/heads/next
> from :0
>
> $ git fast-export master ^next
> reset refs/heads/next
> from :0
>
> Use revs->cmdline instead of revs->pending to iterate over the rev-list
> arguments, checking the UNINTERESTING flag bit to distinguish between
> positive (master, --all, etc) and negative (next.., --not --all, etc)
> revs and avoid enqueueing negative revs in extra_revs.
Use what? You mean, "To solve the problem, lets use".
But this is not correct, cmdline is not being used. Have you even
looked at the patch?
> This does not affect revs that were excluded from the revision walk
> because pointed to by a mark, since those use the SHOWN bit on the
> commit object itself and not UNINTERESTING on the rev_cmdline_entry.
revs? You mean commits?
"excluded because point to by a mark"? Doesn't sound like proper
grammar. Maybe "excluded because they were pointed to by a mark".
And I don't see why this paragraph is needed at all. Why would the
reader think marks have anything to do with this? There's no mention
of marks before.
This might help you, or other people involved in the problem, but not
anybody else. Anything related to marks is completely orthogonal to
this patch, and there's no point in mentioning that.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Jonathan Nieder @ 2012-10-31 1:27 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <CAMP44s3ap19TDsSo_fmNqJp+ROWo2Ka8bc35YQmR3mMw375WsQ@mail.gmail.com>
Felipe Contreras wrote:
> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Felipe Contreras wrote:
>>> No reason to use the full path in case this is used externally.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> "No reason not to" is not a reason to do anything. What symptoms does
>> this prevent? Could you describe the benefit of this patch in a
>> paragraph starting "Now you can ..."?
>
> ./test-lib.sh: line 394:
> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
> No such file or directory
Ok, so a description for this patch is
test: use test's basename to name results file
Running a test using its full path produces an error:
$ ~/dev/git/contrib/remote-hg/test-21865.sh
[...]
./test-lib.sh: line 394: /home/felipec/dev/t/\
test-results//home/felipec/dev/git/contrib/remote-hg/\
test-21865.counts: No such file or directory
In --tee and --valgrind modes we already use the basename
to name the .out and .exit files; this patch teaches the test-lib
to name the .counts file the same way.
That is still not enough to tell if it is a good change, though.
Should the test results for contrib/remote-hg/test-* be included with
the results for t/t*.sh when I run "make aggregate-results"?
Before 60d02ccc, git-svn had its own testsuite under contrib/, with
glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe
that code could provide some inspiration for questions like these.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Sverre Rabbelier @ 2012-10-31 1:33 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Jeff King, Junio C Hamano,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031003721.GV15167@elie.Belkin>
On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>> typename(e->item->type));
>> continue;
>> }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> + * This ref will not be updated through a commit, lets make
>> + * sure it gets properly upddated eventually.
>> + */
>> + if (commit->util || commit->object.flags & SHOWN) {
>> if (!(commit->object.flags & UNINTERESTING))
>> string_list_append(extra_refs, full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>> commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk. It walks through the revs from the
> commandline and for each one:
>
> - peels it to a refname, and then to a commit
> - stores the refname so fast-export knows what arg to pass to
> the "commit" command during the revision walk
> - if it already had a refname stored, instead adds the
> (refname, commit) pair to the extra_refs list, so fast-export
> knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs. That's what this patch does.
>
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
>
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
Thanks for the thorough explanation. Perhaps some of that could make
it's way into the commit message?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-31 1:35 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s3pZsDa8w46JWmxFt=BdrxDxnB_r1p50p7eOiaVcjNs-w@mail.gmail.com>
Felipe Contreras wrote:
> This might help you, or other people involved in the problem, but not
> anybody else.
Ok, I give up. Bye.
Sometimes the author of some code and the right person to interact
with the development community by submitting and maintaining it are
not the same person. Hopefully others more patient than we two can
pick up where we left off.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-31 1:39 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031010823.GX15167@elie.Belkin>
On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> I don't think it's my job to explain to you how 'git fast-export'
>> works.
>
> Actually, if you are submitting a patch for inclusion, it is your job
> to explain to future readers what the patch does.
That's already explained.
> Yes, the reader
> might not be deeply familiar with the part of fast-export you are
> modifying.
This has nothing to do with what you said. I'm literally explaining to
you how 'git fast-export' works in situations that are completely
orthogonal to this patch, because you are using wrong examples as
grounds to prevent this patch from being accepted. It's not my job to
explain to you that 'git fast-export' doesn't work this way, you have
a command line to type those commands and see for yourself if they do
what you think they do with a vanilla version of git. That's exactly
what I did, to make sure I'm not using assumptions as basis for
arguing, it took me a few minutes.
That being said, if your problem is that it's not clear to people not
deeply familiar with that part of fast-export, this extra paragraph in
addition to the current commit message should do the trick:
---
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and
any duplicated ref gets added to a list in order to issue 'reset'
commands after the traversing. Unfortunately, it's not even checking
if the commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
---
And to get that all had to do is ask: "Can you please add an
explanation of what this part of the code does? For the ones of us not
familiar with it".
Not; "This patch looks unsafe", "This patch makes Sally mad", "This
patch causes regressions", and so on.
But hey, at least we are not arguing about what is wrong with this
patch (or so I hope).
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-31 1:51 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s0RcbAiUmvGACxO+H-b-anQSPXxUqUuZwYRKWfrpXYeew@mail.gmail.com>
Felipe Contreras wrote:
> It's not my job to
> explain to you that 'git fast-export' doesn't work this way, you have
> a command line to type those commands and see for yourself if they do
> what you think they do with a vanilla version of git. That's exactly
> what I did, to make sure I'm not using assumptions as basis for
> arguing, it took me a few minutes.
Well no, when I run "git blame" 10 years down the line and do not
understand what your code is doing, it is not at all reasonable to
expect me to checkout the parent commit, get it to compile with a
modern toolchain, and type those commands for myself.
Instead, the commit message should be self-contained and explain what
the patch does.
That has multiple parts:
- first, what the current behavior is
- second, what the intent behind the current behavior is. This is
crucial information because presumably we want the change not to
break that.
- third, what change the patch makes
- fourth, what the consequences of that are, in terms of new use
cases that become possible and old use cases that become less
convenient
- fifth, optionally, how the need for this change was discovered
(real-life usage, code inspection, or something else)
- sixth, optionally, implementation considerations and alternate
approaches that were discarded
If you run "git log", you'll see many good and bad examples to think
over and compare to this goal. It's hard work to describe one's work
well in terms that other people can understand, but I think it can be
satisfying, and in any event, it's just as necessary as including
comments near confusing code.
Sincerely,
Jonathan
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Felipe Contreras @ 2012-10-31 1:59 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <20121031012730.GY15167@elie.Belkin>
On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> > Felipe Contreras wrote:
>
>>>> No reason to use the full path in case this is used externally.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>
>>> "No reason not to" is not a reason to do anything. What symptoms does
>>> this prevent? Could you describe the benefit of this patch in a
>>> paragraph starting "Now you can ..."?
>>
>> ./test-lib.sh: line 394:
>> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts:
>> No such file or directory
>
> Ok, so a description for this patch is
>
> test: use test's basename to name results file
Is this solving an actual problem or is it just something nice to do?
Like in all good novels, one has to read more find out...
> Running a test using its full path produces an error:
I'm not sure what that even means. Do you mean this produces an error?
% make -C t $PWD/t9902-completion.sh
Well, sure it does, but this patch doesn't fix that.
If you want a precise explanation of what kind of usages are enabled
by this patch, that would require some work, and no I haven't done it,
and no, I'm not sure.
> $ ~/dev/git/contrib/remote-hg/test-21865.sh
> [...]
> ./test-lib.sh: line 394: /home/felipec/dev/t/\
> test-results//home/felipec/dev/git/contrib/remote-hg/\
> test-21865.counts: No such file or directory
Except that I didn't do this. So the fact that this happens is an
assumption, and I'm not willing to make that.
Most likely if somebody does that they are doing something wrong; they
didn't define the TESTDIR variable (or something like that).
It's all fun and games to write explanations for things, but it's not
that easy when you want those explanations to be actually true, and
corrent--you have to spend time to make sure of that.
> In --tee and --valgrind modes we already use the basename
> to name the .out and .exit files; this patch teaches the test-lib
> to name the .counts file the same way.
I don't see the point of listing each and every place where this
already happens. As a matter of fact, the base-name is used in other
places as well, and just saying "This is already done in other
places", is more than enough. But who says they are not the ones doing
it wrong? Maybe this part of the code is right, and it's the others
that need fixing. I don't see how saying "Others are doing it" makes
the patch better or worse in any way. There might also be different
reasons for why they do it that doesn't apply here.
> That is still not enough to tell if it is a good change, though.
> Should the test results for contrib/remote-hg/test-* be included with
> the results for t/t*.sh when I run "make aggregate-results"?
>
> Before 60d02ccc, git-svn had its own testsuite under contrib/, with
> glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe
> that code could provide some inspiration for questions like these.
Or maybe they are the ones that should look for inspiration in
contrib/remote-hg.
The patch is obviously correct; it's generally good not to name files
with slashes in them, and $0 is not guaranteed not to have slashes.
Even if you run all the tests inside the 't' directory, this script is
not only used by git, and others might want sub-directories, and not
thousands of tests on the same directory like git.
Either way, if obvious fixes that are one-liners require an essay for
you, I give up.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-31 2:08 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031001117.GA29486@elie.Belkin>
On Wed, Oct 31, 2012 at 1:11 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (cc-ing the git list)
> Felipe Contreras wrote:
>
>> When an object has already been exported (and thus is in the marks) it
>> is flagged as SHOWN, so it will not be exported again, even if this time
>> it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated
>
> Yes, makes perfect sense.
>
> For what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Yay!
> [...]
>> --- a/t/t5800-remote-helpers.sh
>> +++ b/t/t5800-remote-helpers.sh
>> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '
>> compare_refs clone HEAD server refs/heads/new-refspec
>> '
>>
>> +test_expect_success 'push ref with existing object' '
>> + (cd localclone &&
>> + git branch point-to-master master &&
>> + git push origin point-to-master
>> + ) &&
>> +
>> + (cd server &&
>> + git show-ref refs/heads/point-to-master
>> + )
>
> Style: if you indent like this, the test becomes clearer:
And then it would become inconsistent with the rest of the file.
>> + git fast-export --import-marks=tmp-marks \
>> + --export-marks=tmp-marks master > actual &&
>> + test_cmp expected actual
>
> Redirections in git shell scripts are generally spelled as
> "do_something >actual", without a space between the operator and
> filename.
I generally am OK with adapting to whatever code-style is used
(sometimes under protest), but this is a place where I draw the
line. Sorry, '>actual' is more annoying to me than a knife screeching
glass. Fortunately, '> actual' is used in many other places in 't/',
so I'm going to use the other people jumping over the bridge argument.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-31 2:13 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031003721.GV15167@elie.Belkin>
On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>> typename(e->item->type));
>> continue;
>> }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> + * This ref will not be updated through a commit, lets make
>> + * sure it gets properly upddated eventually.
>> + */
>> + if (commit->util || commit->object.flags & SHOWN) {
>> if (!(commit->object.flags & UNINTERESTING))
>> string_list_append(extra_refs, full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>> commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk. It walks through the revs from the
> commandline and for each one:
>
> - peels it to a refname, and then to a commit
> - stores the refname so fast-export knows what arg to pass to
> the "commit" command during the revision walk
> - if it already had a refname stored, instead adds the
> (refname, commit) pair to the extra_refs list, so fast-export
> knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs. That's what this patch does.
That is correct.
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
Maybe, but that's yet another change, and with more changes come more
possibilities of regressions. I haven't verified this is the case.
If this makes sense, I would do it in another, separate patch.
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
I don't know what that means in practice.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Jonathan Nieder @ 2012-10-31 2:13 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <CAMP44s1xAeW2QZsNwRVRx+DEhYVVdiKbw-y-aNuo6unuv7pYZQ@mail.gmail.com>
Felipe Contreras wrote:
> It's all fun and games to write explanations for things, but it's not
> that easy when you want those explanations to be actually true, and
> corrent--you have to spend time to make sure of that.
That's why it's useful for the patch submitter to write them, asking
for help when necessary.
As a bonus, it helps reviewers understand the effect of the patch.
Bugs averted!
[...]
> Either way, if obvious fixes that are one-liners require an essay for
> you, I give up.
I guess it is fair to call a reasonable subject line plus a couple of
sentences an essay. Yes, obvious fixes especially require that, since
the bug caused by an obvious fix is one of the worst kinds.
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-31 2:22 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121031015103.GA15167@elie.Belkin>
On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It's not my job to
>> explain to you that 'git fast-export' doesn't work this way, you have
>> a command line to type those commands and see for yourself if they do
>> what you think they do with a vanilla version of git. That's exactly
>> what I did, to make sure I'm not using assumptions as basis for
>> arguing, it took me a few minutes.
>
> Well no, when I run "git blame" 10 years down the line and do not
> understand what your code is doing, it is not at all reasonable to
> expect me to checkout the parent commit, get it to compile with a
> modern toolchain, and type those commands for myself.
>
> Instead, the commit message should be self-contained and explain what
> the patch does.
>
> That has multiple parts:
>
> - first, what the current behavior is
>
> - second, what the intent behind the current behavior is. This is
> crucial information because presumably we want the change not to
> break that.
>
> - third, what change the patch makes
>
> - fourth, what the consequences of that are, in terms of new use
> cases that become possible and old use cases that become less
> convenient
>
> - fifth, optionally, how the need for this change was discovered
> (real-life usage, code inspection, or something else)
>
> - sixth, optionally, implementation considerations and alternate
> approaches that were discarded
I don't see any "Explain in detail what different commands do, even if
they are irrelevant to the patch in question because someone might
think they would get broken by this patch when in fact they wouldn't",
that might belong in the discussion, but not in the commit message,
and certainly not in the form of any entitlement.
Again, it's _your_ responsibility to make sure the commands you say
might get broken do actually work with your current git, it's not mine
to run them for you, even though that's exactly what I did, because
I'm interested in getting things correctly on record.
And FTR, since you removed it, here is what I proposed to add to the
commit message:
---
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and
any duplicated ref gets added to a list in order to issue 'reset'
commands after the traversing. Unfortunately, it's not even checking
if the commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
---
With that, all the points above are tackled, except fourth, because
there aren't any.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Felipe Contreras @ 2012-10-31 2:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Junio C Hamano, Jeff King, Ævar Arnfjörð,
Johannes Sixt
In-Reply-To: <20121031021318.GB15167@elie.Belkin>
On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> It's all fun and games to write explanations for things, but it's not
>> that easy when you want those explanations to be actually true, and
>> corrent--you have to spend time to make sure of that.
>
> That's why it's useful for the patch submitter to write them, asking
> for help when necessary.
>
> As a bonus, it helps reviewers understand the effect of the patch.
> Bugs averted!
Yeah, that would be nice. Too bad I don't have that information, and
have _zero_ motivation to go and get it for you.
> [...]
>> Either way, if obvious fixes that are one-liners require an essay for
>> you, I give up.
>
> I guess it is fair to call a reasonable subject line plus a couple of
> sentences an essay. Yes, obvious fixes especially require that, since
> the bug caused by an obvious fix is one of the worst kinds.
Yes, I've written essays for one-line fixes, in the Linux kernel,
where details matter, and things are not so obvious.
This is not the case here. This is as obvious and simple as things
get. If there was a problem with it, somebody would have found it by
now.
Let not the perfect be the enemy of the good. Or do. Up to you.
--
Felipe Contreras
^ permalink raw reply
* [PATCH] git-push: update remote tags only with force
From: Chris Rorvick @ 2012-10-31 5:37 UTC (permalink / raw)
To: git
References are allowed to update from one commit-ish to another if the
former is a ancestor of the latter. This behavior is oriented to
branches which are expected to move with commits. Tag references are
expected to be static in a repository, though, thus an update to a
tag (lightweight and annotated) should be rejected unless the update is
forced.
To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:
1) The old and new references must be commits. If this fails,
it is not a valid update for a branch.
2) The reference name cannot start with "refs/tags/". This
catches lightweight tags which (usually) point to commits
and therefore would not be caught by (1).
If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote. This can be overridden by passing
--force to git push.
The new status has the added benefit of being able to provide accurate
feedback as to why the ref update failed and what can be done.
Currently all ref update rejections are assumed to be for branches.
Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
Documentation/git-push.txt | 10 +++++-----
builtin/push.c | 12 ++++++++++++
builtin/send-pack.c | 6 ++++++
cache.h | 3 +++
remote.c | 34 +++++++++++++++++++++++++++-------
t/t5516-fetch-push.sh | 30 +++++++++++++++++++++++++++++-
transport-helper.c | 6 ++++++
transport.c | 25 ++++++++++++++++---------
transport.h | 3 ++-
9 files changed, 106 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 22d2580..7107d76 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
updated.
+
The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>. By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward. This does *not* attempt to merge <src> into <dst>. See
-EXAMPLES below for details.
+on the remote side. By default this is only allowed if the update is
+a branch, and then only if it can fast-forward <dst>. By having the
+optional leading `+`, you can tell git to update the <dst> ref even when
+the update is not a branch or it is not a fast-forward. This does *not*
+attempt to merge <src> into <dst>. See EXAMPLES below for details.
+
`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
+
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..fabcea0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =
"(e.g. 'git pull') before pushing again.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
+static const char message_advice_ref_already_exists[] =
+ N_("Updates were rejected because a matching reference already exists in\n"
+ "the remote. Use git push -f if you really want to make this update.");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)
advise(_(message_advice_checkout_pull_push));
}
+static void advise_ref_already_exists(void)
+{
+ advise(_(message_advice_ref_already_exists));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -277,6 +286,9 @@ static int push_with_options(struct transport *transport, int flags)
else
advise_checkout_pull_push();
break;
+ case ALREADY_EXISTS:
+ advise_ref_already_exists();
+ break;
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d05064..f159ec3 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -202,6 +202,11 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
+ res = "error";
+ msg = "already exists";
+ break;
+
case REF_STATUS_REJECT_NODELETE:
case REF_STATUS_REMOTE_REJECT:
res = "error";
@@ -288,6 +293,7 @@ int send_pack(struct send_pack_args *args,
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/cache.h b/cache.h
index a58df84..2d160a9 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,11 +1002,14 @@ struct ref {
unsigned int force:1,
merge:1,
nonfastforward:1,
+ forwardable:1,
+ update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
REF_STATUS_OK,
REF_STATUS_REJECT_NONFASTFORWARD,
+ REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
diff --git a/remote.c b/remote.c
index 04fd9ea..0d94888 100644
--- a/remote.c
+++ b/remote.c
@@ -1309,22 +1309,42 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* to overwrite it; you would not know what you are losing
* otherwise.
*
- * (3) if both new and old are commit-ish, and new is a
- * descendant of old, it is OK.
+ * (3) if both new and old are commits, the reference is not
+ * a tag, and new is a descendant of old, it is OK.
*
* (4) regardless of all of the above, removing :B is
* always allowed.
*/
- ref->nonfastforward =
+ ref->update =
!ref->deletion &&
- !is_null_sha1(ref->old_sha1) &&
+ !is_null_sha1(ref->old_sha1);
+
+ ref->nonfastforward =
+ ref->update &&
(!has_sha1_file(ref->old_sha1)
|| !ref_newer(ref->new_sha1, ref->old_sha1));
- if (ref->nonfastforward && !ref->force && !force_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
+ if (prefixcmp(ref->name, "refs/tags/")) {
+ struct object *old = parse_object(ref->old_sha1);
+ struct object *new = parse_object(ref->new_sha1);
+ ref->forwardable = (old && new &&
+ old->type == OBJ_COMMIT && new->type == OBJ_COMMIT);
+ } else
+ ref->forwardable = 0;
+
+ if (!ref->force && !force_update) {
+ if (ref->forwardable) {
+ if (ref->nonfastforward) {
+ ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+ continue;
+ }
+ } else {
+ if (ref->update) {
+ ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ continue;
+ }
+ }
}
}
}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..cff559f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '
git branch -D frotz
fi &&
git tag -f frotz &&
- git push testrepo frotz &&
+ git push -f testrepo frotz &&
check_push_result $the_commit tags/frotz &&
check_push_result $the_first_commit heads/frotz
@@ -929,6 +929,34 @@ test_expect_success 'push into aliased refs (inconsistent)' '
)
'
+test_expect_success 'push tag requires --force to update remote tag' '
+ mk_test heads/master &&
+ mk_child child1 &&
+ mk_child child2 &&
+ (
+ cd child1 &&
+ git tag lw_tag &&
+ git tag -a -m "message 1" ann_tag &&
+ git push ../child2 lw_tag &&
+ git push ../child2 ann_tag &&
+ >file1 &&
+ git add file1 &&
+ git commit -m "file1" &&
+ git tag -f lw_tag &&
+ git tag -f -a -m "message 2" ann_tag &&
+ ! git push ../child2 lw_tag &&
+ ! git push ../child2 ann_tag &&
+ git push --force ../child2 lw_tag &&
+ git push --force ../child2 ann_tag &&
+ git tag -f lw_tag HEAD~ &&
+ git tag -f -a -m "message 3" ann_tag &&
+ ! git push ../child2 lw_tag &&
+ ! git push ../child2 ann_tag &&
+ git push --force ../child2 lw_tag &&
+ git push --force ../child2 ann_tag
+ )
+'
+
test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo "To testrepo" &&
diff --git a/transport-helper.c b/transport-helper.c
index cfe0988..ef9a6f8 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -643,6 +643,11 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "already exists")) {
+ status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
@@ -702,6 +707,7 @@ static int push_refs_with_push(struct transport *transport,
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport.c b/transport.c
index 9932f40..d218884 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
const char *msg;
strcpy(quickref, status_abbrev(ref->old_sha1));
- if (ref->nonfastforward) {
+ if (ref->nonfastforward || (!ref->forwardable && ref->update)) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"non-fast-forward", porcelain);
break;
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "already exists", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -714,7 +718,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
}
void transport_print_push_status(const char *dest, struct ref *refs,
- int verbose, int porcelain, int *nonfastforward)
+ int verbose, int porcelain, int *willnotupdate)
{
struct ref *ref;
int n = 0;
@@ -733,18 +737,21 @@ void transport_print_push_status(const char *dest, struct ref *refs,
if (ref->status == REF_STATUS_OK)
n += print_one_push_status(ref, dest, n, porcelain);
- *nonfastforward = 0;
+ *willnotupdate = 0;
for (ref = refs; ref; ref = ref->next) {
if (ref->status != REF_STATUS_NONE &&
ref->status != REF_STATUS_UPTODATE &&
ref->status != REF_STATUS_OK)
n += print_one_push_status(ref, dest, n, porcelain);
if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
- *nonfastforward != NON_FF_HEAD) {
+ *willnotupdate != NON_FF_HEAD) {
if (!strcmp(head, ref->name))
- *nonfastforward = NON_FF_HEAD;
+ *willnotupdate = NON_FF_HEAD;
else
- *nonfastforward = NON_FF_OTHER;
+ *willnotupdate = NON_FF_OTHER;
+ } else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS &&
+ *willnotupdate == 0) {
+ *willnotupdate = ALREADY_EXISTS;
}
}
}
@@ -1031,9 +1038,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
int transport_push(struct transport *transport,
int refspec_nr, const char **refspec, int flags,
- int *nonfastforward)
+ int *willnotupdate)
{
- *nonfastforward = 0;
+ *willnotupdate = 0;
transport_verify_remote_names(refspec_nr, refspec);
if (transport->push) {
@@ -1099,7 +1106,7 @@ int transport_push(struct transport *transport,
if (!quiet || err)
transport_print_push_status(transport->url, remote_refs,
verbose | porcelain, porcelain,
- nonfastforward);
+ willnotupdate);
if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index 3b21c4a..326271e 100644
--- a/transport.h
+++ b/transport.h
@@ -142,6 +142,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define NON_FF_HEAD 1
#define NON_FF_OTHER 2
+#define ALREADY_EXISTS 3
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
int * nonfastforward);
@@ -170,7 +171,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
int transport_refs_pushed(struct ref *ref);
void transport_print_push_status(const char *dest, struct ref *refs,
- int verbose, int porcelain, int *nonfastforward);
+ int verbose, int porcelain, int *willnotupdate);
typedef void alternate_ref_fn(const struct ref *, void *);
extern void for_each_alternate_ref(alternate_ref_fn, void *);
--
1.8.0.1.ga7c0bae
^ permalink raw reply related
* Re: [PATCH] git-push: update remote tags only with force
From: Felipe Contreras @ 2012-10-31 5:55 UTC (permalink / raw)
To: Chris Rorvick; +Cc: git
In-Reply-To: <1351661875-4307-1-git-send-email-chris@rorvick.com>
Hi,
(again because the mailing list rejected it) (Gmal switched interface
and HTML is the default)
On Wed, Oct 31, 2012 at 6:37 AM, Chris Rorvick <chris@rorvick.com> wrote:
>
> References are allowed to update from one commit-ish to another if the
> former is a ancestor of the latter. This behavior is oriented to
> branches which are expected to move with commits. Tag references are
> expected to be static in a repository, though, thus an update to a
> tag (lightweight and annotated) should be rejected unless the update is
> forced.
>
> To enable this functionality, the following checks have been added to
> set_ref_status_for_push() for updating refs (i.e, not new or deletion)
> to restrict fast-forwarding in pushes:
>
> 1) The old and new references must be commits. If this fails,
> it is not a valid update for a branch.
>
> 2) The reference name cannot start with "refs/tags/". This
> catches lightweight tags which (usually) point to commits
> and therefore would not be caught by (1).
>
> If either of these checks fails, then it is flagged (by default) with a
> status indicating the update is being rejected due to the reference
> already existing in the remote. This can be overridden by passing
> --force to git push.
>
> The new status has the added benefit of being able to provide accurate
> feedback as to why the ref update failed and what can be done.
> Currently all ref update rejections are assumed to be for branches.
Makes sense to me. I've believe I've been hit by this a couple of
times when tags were updated, and a colleague did 'git push' and they
went all back, or something like that. To handle that case properly
probably more changes are needed, but this is a change in the right
direction.
> +test_expect_success 'push tag requires --force to update remote tag' '
> + mk_test heads/master &&
> + mk_child child1 &&
> + mk_child child2 &&
> + (
> + cd child1 &&
> + git tag lw_tag &&
> + git tag -a -m "message 1" ann_tag &&
> + git push ../child2 lw_tag &&
> + git push ../child2 ann_tag &&
> + >file1 &&
> + git add file1 &&
> + git commit -m "file1" &&
> + git tag -f lw_tag &&
> + git tag -f -a -m "message 2" ann_tag &&
> + ! git push ../child2 lw_tag &&
You probably should use test_must_fail.
I don't see anything wrong with the patch, but I wonder if it might be
possible to split it to ease the review.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
From: Jonathan Nieder @ 2012-10-31 6:05 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Felipe Contreras, git, Jeff King, Junio C Hamano,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAGdFq_jNM_48muXJ0BX2ehC=k8T9GLui_QtRO8D8C7h6b5jyHg@mail.gmail.com>
Sverre Rabbelier wrote:
> Thanks for the thorough explanation. Perhaps some of that could make
> it's way into the commit message?
It's fine with me if it doesn't, since the original commit message
covers the basics (current behavior and intent of the change) in its
first two paragraphs and anyone wanting more detail can use
GIT_NOTES_REF=refs/remotes/charon/notes/full \
git show --show-notes <commit>
to find more details.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'
From: Johannes Sixt @ 2012-10-31 6:29 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Jeff King
In-Reply-To: <50904677.2020308@web.de>
Am 10/30/2012 22:28, schrieb Jens Lehmann:
> Am 29.10.2012 08:11, schrieb Johannes Sixt:
>> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>>> + /* Remove trailing '/' from directories to find submodules in the index */
>>> + for (i = 0; i < argc; i++) {
>>> + size_t pathlen = strlen(argv[i]);
>>> + if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))
>>> + argv[i] = xmemdupz(argv[i], pathlen - 1);
>>> + }
>>> +
>>> pathspec = get_pathspec(prefix, argv);
>>> refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
>>
>> That's wrong: Either move the check below get_pathspec() (which normalizes
>> backslashes to forward-slashes on Windows) or use is_dir_sep().
>
> Thanks for bringing this up.
>
>> But isn't it somewhat dangerous to check pathspec for existance in the
>> worktree without interpreting them? Think of magic pathspec syntax (that
>> we do not have yet, but which may materialize sometime in the future).
>
> I have to admit I'm not aware of magic pathspec syntax. Do you happen to
> have any pointers where I could look at code doing similar things right?
cmd_mv() in builtin/mv.c looks like a good candidate. It has to check
whether the destination (the last argument) is a directory.
-- Hannes
^ permalink raw reply
* Re: Can't understand the behaviour of git-diff --submodule
From: Francis Moreau @ 2012-10-31 7:36 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git
In-Reply-To: <509043DA.6040606@web.de>
Hi,
On Tue, Oct 30, 2012 at 10:17 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Am 28.10.2012 01:02, schrieb Jens Lehmann:
>> Am 26.10.2012 22:43, schrieb Francis Moreau:
>>> On Fri, Oct 26, 2012 at 10:05 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>> [...]
>>>>
>>>> That is weird, "git diff --submodule" should show that too. Is there
>>>> anything unusual about your setup? (The only explanation I can come
>>>> up with after checking the code is that your submodule has neither a
>>>> .git directory nor a gitfile or the objects directory in there doesn't
>>>> contain these commits)
>>>
>>> Oh now you're asking, I think the submodule has been added by using
>>> the --reference option of git-submodule-add.
>>>
>>> $ cd configs
>>> $ cat .git
>>> gitdir: ../.git/modules/configs
>>
>> Thanks, I suspect the --reference option makes the difference here,
>> I'll check that as soon as I find some time.
>
> Since 1.7.11 and 1.7.10.3 git does handle submodules with alternates
> (which is what --reference uses) correctly. What version are you
> seeing this problem with?
git version 1.7.10.4
Thanks.
--
Francis
^ permalink raw reply
* Re: [PATCH] test-lib: avoid full path to store test results
From: Stefano Lattarini @ 2012-10-31 9:05 UTC (permalink / raw)
To: Elia Pinto
Cc: Jonathan Nieder, Felipe Contreras, git, Junio C Hamano, Jeff King,
Ævar Arnfjörð, Johannes Sixt
In-Reply-To: <CA+EOSBmTjwmf+dO-dgU+rGQaVEKDZw7u9ujrh5jYZkPM2zisOA@mail.gmail.com>
On 10/30/2012 11:17 PM, Elia Pinto wrote:
> Thanks. I know that posix support these usages, but exists some
> traditional shell that not support it.
>
True, but those shells are not POSIX shells -- the major example that
comes to mind is the accursed Solaris /bin/sh.
Since Git assumes a POSIX shell in its scripts and testsuite, use of
any POSIX feature should be fine -- until someone can show a real-world
POSIX shell that (likely due to a bug) fails to grasp such feature, in
which case a "pragmatic" workaround is needed.
Oh, and BTW, there are talks (and mostly consensus) among the Autotools
developers to start requiring a POSIX shell in the configure scripts
and Makefile recipes in the near future:
<http://lists.gnu.org/archive/html/bug-autoconf/2012-06/msg00009.html>
And also, related:
<http://lists.gnu.org/archive/html/automake/2012-08/msg00046.html>
<http://lists.gnu.org/archive/html/coreutils/2012-10/msg00127.html>
>These are described in the
> autoconf manual, last time i have checked. As the construct ; export
> var = x should be portable, but it is not.
>
I don't think POSIX requires that to be portable.
> If this is important these days i don't know.
>
I hope the above helps to clarify the matter a little.
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Michael J Gruber @ 2012-10-31 9:30 UTC (permalink / raw)
To: Felipe Contreras
Cc: Johannes Schindelin, Jeff King, git, Junio C Hamano,
Sverre Rabbelier, Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <CAMP44s0akZ7_Nd1Q1AaZJuXnyTJv2MzNqDus76Y82y4LbWVO+Q@mail.gmail.com>
[quotes heavily cut down by me]
Felipe Contreras venit, vidit, dixit 30.10.2012 21:15:
> Hi,
>
> On Tue, Oct 30, 2012 at 8:33 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> On Tue, 30 Oct 2012, Felipe Contreras wrote:
>>
>>> But you mentioned something about cooperation, and I've yet to see how
>>> is it that you are planning to cooperate. If you say you don't have time
>>> to spend on this, I don't see why I should worry about testing this
>>> series of patches.
>>
>> It has been mentioned before that the communication style including all
>> these snarky and nasty comments is not helpful.
>
For the record, Johannes is not the only one being kept from looking at
this series (further) by the tone of this discussion. Per hominem
attacks are neither professional nor helpful. We prefer to discuss code
here, just code. From my comments on an earlier version of your series
you can see I've tried. The way other comment threads on this series
unfolded made me choose to be a mere by-stander again.
>> and I've yet to see how is it that you are planning to cooperate.
>
> This is also a fact. You haven't provided a branch, you haven't reviewed
> my implementation, you haven't tried it. You mentioned something about
This does not become true through iteration. Max' recent post 'On
git-remote-hg (the "native" one)' [1] points at the msysgit wiki on
remote-hg [2] and his remote-hg branch [3], which is based on and points
at Sverre's original branch [4] and mine [5] which is [4] being
regularly rebased on origin/next. The msysgit devel branch is in heavy
use; I don't use mine often but run the test suite on every rebase
before pushing out.
If the issues that Sverre and Dscho tried to address with their git.git
core (non-helper) patches turn out to be non-issues then I assume
everyone will be happy, including them. You and they have thought a lot
about these things and the way hg-git sync can work. There seems to be
diagreement about the way fast-export/the remote helpers communicate
which revs and refs that are to be synced and updated. This is not
hg-specific, and I suggest to try and clarify that issue as thoroughly
and calmly as possible. Everyone will benefit, and it will make clearer
which tests are appropriate, and accordingly which fixes fix real problems.
Orthogonal to this, it seems that all hg-git interfaces could take
advantage of a "git heads" feature if we resurrect the old ideas (can't
find the thread right now).
Hoping for the best,
Michael
[1] http://permalink.gmane.org/gmane.comp.version-control.git/201083
[2] https://github.com/msysgit/msysgit/wiki/Guide-to-git-remote-hg
[3] https://github.com/fingolfin/git/tree/remote-hg
[4] https://github.com/SRabbelier/git/tree/remote-hg
[5] https://github.com/mjg/git/tree/remote-hg
^ permalink raw reply
* [OT] How to get the discussion details via notes
From: Peter Baumann @ 2012-10-31 9:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
In-Reply-To: <20121031060529.GA30432@elie.Belkin>
Dropping the Cc list, as this is off topic
On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
> Sverre Rabbelier wrote:
>
> > Thanks for the thorough explanation. Perhaps some of that could make
> > it's way into the commit message?
>
> It's fine with me if it doesn't, since the original commit message
> covers the basics (current behavior and intent of the change) in its
> first two paragraphs and anyone wanting more detail can use
>
> GIT_NOTES_REF=refs/remotes/charon/notes/full \
> git show --show-notes <commit>
>
> to find more details.
I seem to miss something here, but I don't get it how the notes ref
becomes magically filled with the details of this discussion.
Care to explain?
-Peter
^ permalink raw reply
* Re: [PATCH v4 00/13] New remote-hg helper
From: Jeff King @ 2012-10-31 10:27 UTC (permalink / raw)
To: git
Cc: Michael J Gruber, Felipe Contreras, Johannes Schindelin,
Junio C Hamano, Sverre Rabbelier, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <5090EFCA.7070606@drmicha.warpmail.net>
On Wed, Oct 31, 2012 at 10:30:50AM +0100, Michael J Gruber wrote:
> For the record, Johannes is not the only one being kept from looking at
> this series (further) by the tone of this discussion. Per hominem
> attacks are neither professional nor helpful. We prefer to discuss code
> here, just code. From my comments on an earlier version of your series
> you can see I've tried. The way other comment threads on this series
> unfolded made me choose to be a mere by-stander again.
Me too. I really like some of the directions the series is taking, and
as the maintainer, I'd like to pick it up. But there is a big question
mark for me still about how it relates to the work in msysgit,
especially:
- What advantages does this implementation have over the one in
msysgit (i.e., new features that the other one does not have)?
- What disadvantages? If this implementation goes into git.git,
the msysgit one is likely to wane in popularity. What will we be
losing by doing so? If the answer is not "nothing", how hard would
it be to port over the missing bits?
- The msysgit one got held up by fixes needed for fast-export. Why
aren't those a problem for this implementation? If we are using a
different strategy that avoids the issue, what are the limitations
(if any) of that strategy?
I have a feeling that some of those answers are buried deep within the
discussion, but I have had a hard time following all of the back and
forth due to the volume and tone of the discussion. Are we at a point
now where some of the participants can try to summarize the situation?
I am not saying that this implementation must be 100% better than the
msysgit one. I do not want perfect to to be the enemy of good and end up
with nothing. But at the same time, there really are two competing
implementations, one of which has received substantially more field use.
Even though the msysgit one is not in git.git, it seems like the path
for making it happen exists (even if it has not been followed yet).
Before merging an alternative implementation, I would want to know what
we are potentially throwing away from the msysgit side, and make sure
that we are not following a wrong path that msysgit has already tried
and found to be lacking.
-Peff
^ 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