* Re: [PATCH] test-lib: avoid full path to store test results
From: Elia Pinto @ 2012-10-30 22:17 UTC (permalink / raw)
To: Jonathan Nieder, Felipe Contreras, git, Junio C Hamano, Jeff King,
Ævar Arnfjörð, Johannes Sixt
In-Reply-To: <20121030070152.GA13324@elie.Belkin>
Thanks. I know that posix support these usages, but exists some
traditional shell that not support it. 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. If this is important these
days i don't know.
Best
2012/10/30, Jonathan Nieder <jrnieder@gmail.com>:
> Elia Pinto wrote:
>
>> The shell word splitting done in base is a bashism, iow not portable.
>
> No, ${varname##glob} is in POSIX and we already use it here and there.
> See Documentation/CodingGuidelines:
>
> - We use ${parameter#word} and its [#%] siblings, and their
> doubled "longest matching" form.
>
> Thanks for looking the patch over.
> Jonathan
>
--
Inviato dal mio dispositivo mobile
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-30 22:18 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: >, Jeff King, Junio C Hamano, Jonathan Nieder,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAGdFq_h3L-1rPvb=dSYeXqEea+f+g2kRHp7aAjaU-AxjZHB7dQ@mail.gmail.com>
On Tue, Oct 30, 2012 at 10:59 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> Why would it? We are not changing the way objects are exported, the
>>>> only difference is what happens at the end
>>>> (handle_tags_and_duplicates()).
>>>
>>> Because the marking is per-commit, not per-ref, right?
>>
>> Oh, you meant using marks?
>
> No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per
> ref? That is, commit->object.flags & SHOWN refers to the object
> underlying the ref. So I suspect this scenario doesn't pass the tests:
Without marks you cannot have the SHOWN mark at that point; we haven't
traversed the commits.
> git init &&
> echo first > content &&
> git add content &&
> git commit -m "first" &&
> git branch first &&
> echo two > content &&
> git commit -m "second" &&
> git branch second &&
> git fast-export first > actual &&
> test_cmp actual expected_first &&
> git fast-export second > actual &&
> test_cmp actual expected_second
>
> With expected_first being something like:
> <fast-export stream with the first commit>
> <reset command to set first to the right commit>
Why would a 'reset' command be expected if the 'first' branch is
already pointing to the 'first' commit?
> And expected_second being something like
> <fast export stream with the first and second command>
> <reset command to set first and second to their respective branches>
Ditto, plus, why would 'git fast-export second' do anything regarding
'first'? It wasn't specified in the committish; it's not relevant.
Before an after my patch the output is the same:
% git fast-export first:
reset refs/heads/first
commit refs/heads/first
% git fast-export second:
reset refs/heads/second
commit refs/heads/second
commit refs/heads/second
Which is expected and correct; the branch already points to the right
commit, no need for an extra reset.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: change symlink
From: shawn wilson @ 2012-10-30 22:22 UTC (permalink / raw)
To: Andreas Schwab; +Cc: git
In-Reply-To: <m2390vobnh.fsf@igel.home>
On Tue, Oct 30, 2012 at 10:09 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> shawn wilson <ag4ve.us@gmail.com> writes:
>
>> and once it's added, status says:
>> # renamed: t2 -> t2/one/test
>>
>> that's not exactly true, but...
>
> What's wrong with it? Both files have the same contents, which is the
> link target for symlinks.
>
ok, you're right about that (another test where i changed where the
symlink pointed):
# deleted: test
# new file: test/one/t3
however (what got me started wondering about this and a point i forgot
about) - t2/one/test doesn't show up under 'untracked files' in in
status that scenario. shouldn't it?
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Felipe Contreras @ 2012-10-30 22:22 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <20121030220717.GO15167@elie.Belkin>
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Well, that's what we have now, and you want to preserve this "feature"
>> (aka bug), right?
>
> 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).
> I also think the proposed behavior is wrong.
That's a different matter, lets see what others think.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Sverre Rabbelier @ 2012-10-30 22:35 UTC (permalink / raw)
To: Felipe Contreras
Cc: >, Jeff King, Junio C Hamano, Jonathan Nieder,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s2KNmr7zAvFo2gOR8G=YaoBWiGPCjPY47x00eev6MOAFw@mail.gmail.com>
On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Which is expected and correct; the branch already points to the right
> commit, no need for an extra reset.
I think you're correct. Thanks for confirming.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 1/2] completion: refactor __gitcomp related tests
From: SZEDER Gábor @ 2012-10-30 22:45 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <1350869941-22485-2-git-send-email-felipe.contreras@gmail.com>
On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:
> Lots of duplicated code!
>
> No functional changes.
I'm not sure.
I'm all for removing duplicated application code, but I'm usually more
conservative when it comes to test code. The more logic, the more
possibility for bugs in tests. So tests should be dead simple, even
if that means some duplicated test code or the lack of convenience
functions.
While this might be considered just a matter of personal preference, ...
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> t/t9902-completion.sh | 72 ++++++++++++++++-----------------------------------
> 1 file changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cbd0fb6..1c6952a 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -72,87 +72,61 @@ test_completion_long ()
>
> newline=$'\n'
>
> -test_expect_success '__gitcomp - trailing space - options' '
> - sed -e "s/Z$//" >expected <<-\EOF &&
> - --reuse-message=Z
> - --reedit-message=Z
> - --reset-author Z
> - EOF
> +test_gitcomp ()
> +{
> + sed -e 's/Z$//' > expected &&
> (
> local -a COMPREPLY &&
> - cur="--re" &&
> - __gitcomp "--dry-run --reuse-message= --reedit-message=
> - --reset-author" &&
> + cur="$1" &&
> + shift &&
> + __gitcomp "$@" &&
... I was really puzzled by how __gitcomp() gets its arguments here,
and had to think for a while to figure out why it's not broken.
^ permalink raw reply
* Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance
From: Albert Krawczyk @ 2012-10-30 22:50 UTC (permalink / raw)
To: kusmabite; +Cc: karsten.blees, git, msysgit
In-Reply-To: <CABPQNSb6QzxxJpoackhzRz19UWwpxjJ6MPEeptTU9Z0bEaAB+A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]
> Wow, awesome results :)
They get even better when used with scripted commands like git svn rebase:
| cold | warm | hot
----------------+-------+------+-----
msysgit-v1.8.0 | 7m29s | 36s | 33s
this patch | 28s | 14s | 14
https://github.com/pro-logic/git/commit/32c03dd8#commitcomment-2070415
Karsten's code is amazing. Now if only I could figure out a way to spread
the love through the rest of the source :)
Albert
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
[-- Attachment #2: Type: text/html, Size: 1546 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Felipe Contreras @ 2012-10-30 22:56 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: >, Jeff King, Junio C Hamano, Jonathan Nieder,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAGdFq_iiGpYW-txPaa6mZrxg3mYdOX-Ez9uLF-rB5bAjZd5rWg@mail.gmail.com>
On Tue, Oct 30, 2012 at 11:35 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Which is expected and correct; the branch already points to the right
>> commit, no need for an extra reset.
>
> I think you're correct. Thanks for confirming.
Thanks for reviewing. If you are still not convinced, I could pull the
patches from msysgit and simplify them, I'm sure the end result would
be pretty similar, if not exactly the same as this patch (plus other
orthogonal changes). I saw some patches that were not part of the
patch series you sent before, so maybe that's why you expected certain
behavior that wasn't actually there in that particular patch series.
But hopefully that's not needed.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v6 2/3] completion: add new __gitcompadd helper
From: SZEDER Gábor @ 2012-10-30 22:58 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano, Jeff King, Matthieu Moy
In-Reply-To: <1350870342-22653-3-git-send-email-felipe.contreras@gmail.com>
On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote:
> The idea is to never touch the COMPREPLY variable directly.
>
> This allows other completion systems override __gitcompadd, and do
> something different instead.
>
> Also, this allows the simplification of the completion tests (separate
> patch).
This doesn't apply anymore, does it? The mentioned simplification is
done in the other series.
^ permalink raw reply
* Re: [PATCH 1/2] completion: refactor __gitcomp related tests
From: Felipe Contreras @ 2012-10-30 23:02 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <20121030224558.GN12052@goldbirke>
On Tue, Oct 30, 2012 at 11:45 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:
>> Lots of duplicated code!
>>
>> No functional changes.
>
> I'm not sure.
> I'm all for removing duplicated application code, but I'm usually more
> conservative when it comes to test code. The more logic, the more
> possibility for bugs in tests. So tests should be dead simple, even
> if that means some duplicated test code or the lack of convenience
> functions.
> While this might be considered just a matter of personal preference, ...
Fair enough, but my preference is different: test code should be the
same as normal code; easy to maintain.
>> +test_gitcomp ()
>> +{
>> + sed -e 's/Z$//' > expected &&
>> (
>> local -a COMPREPLY &&
>> - cur="--re" &&
>> - __gitcomp "--dry-run --reuse-message= --reedit-message=
>> - --reset-author" &&
>> + cur="$1" &&
>> + shift &&
>> + __gitcomp "$@" &&
>
> ... I was really puzzled by how __gitcomp() gets its arguments here,
> and had to think for a while to figure out why it's not broken.
You mean because "$@" is treated in a special way? Without that
behavior I'm not sure many things would be possible :)
Anyway, I don't think we should make our life more difficult while
maintaining test code... that's my opinion.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v6 2/3] completion: add new __gitcompadd helper
From: Felipe Contreras @ 2012-10-30 23:05 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Jeff King, Matthieu Moy
In-Reply-To: <20121030225839.GO12052@goldbirke>
On Tue, Oct 30, 2012 at 11:58 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote:
>> The idea is to never touch the COMPREPLY variable directly.
>>
>> This allows other completion systems override __gitcompadd, and do
>> something different instead.
>>
>> Also, this allows the simplification of the completion tests (separate
>> patch).
>
> This doesn't apply anymore, does it? The mentioned simplification is
> done in the other series.
Yeah, but you mentioned you didn't like all the COMPREPLY=() changes
and it might be time to get rid of them.
So this series supersedes that one.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH v5 1/3] completion: add new __gitcompadd helper
From: SZEDER Gábor @ 2012-10-30 23:18 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <CAMP44s3guAhFe1ACrAzC9T05fCMAejU6Wn+GtYfSpmfJ8EX-xg@mail.gmail.com>
On Mon, Oct 22, 2012 at 02:41:21AM +0200, Felipe Contreras wrote:
> On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
>
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index d743e56..01325de 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
> >> fi
> >> fi
> >>
> >> +__gitcompadd ()
> >> +{
> >> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
> >> +}
> >> +
> >> # Generates completion reply with compgen, appending a space to possible
> >> # completion words, if necessary.
> >> # It accepts 1 to 4 arguments:
> >> @@ -238,13 +243,11 @@ __gitcomp ()
> >>
> >> case "$cur_" in
> >> --*=)
> >> - COMPREPLY=()
> >> + __gitcompadd
> >> ;;
> >> *)
> >> local IFS=$'\n'
> >> - COMPREPLY=($(compgen -P "${2-}" \
> >> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> >> - -- "$cur_"))
> >> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""
> >> ;;
> >> esac
> >> }
> >> @@ -261,7 +264,7 @@ __gitcomp ()
> >> __gitcomp_nl ()
> >> {
> >> local IFS=$'\n'
> >> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> >> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
> >> }
> >
> > I feel hesitant about this change. One of the ways I'm exploring to
> > fix the issues with shell metacharacters and expansion in compgen is
> > to actually replace compgen. We already iterate over all possible
> > completion words in __gitcomp_1(), so it doesn't make much of a
> > difference to do the filtering for the current word while we are at
> > it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
> > ...)), and tha basic idea of never touching COMPREPLY directly make
> > this basically impossible.
>
> How is it impossible? You can still replace compgen, all you have to
> do is modify __gitcompadd and replace that code with whatever custom
> code you want. You can change the arguments and everything. The only
> limitation is that it should be the only place where COMPREPLY is
> modified, and all is good. Well, it doesn't have to be only _one_
> place, but the less functions that do this, the better.
That's exactly the problem: there isn't, there can't be one single
"whatever custom code I want".
The compgen() in __gitcomp() will be replaced by an enhanced version
of the loop in __gitcomp_1(), while in __gitcomp_nl() it will be
replaced by a little awk scriptlet. And then there is the oddball
$(git ls-tree |sed magic) in __git_complete_revlist_file(), where
possible completion words are filenames possibly containing newlines,
therefore requiring yet another approach.
^ permalink raw reply
* Re: Links broken in ref docs.
From: Mike Norman @ 2012-10-30 23:30 UTC (permalink / raw)
To: Holger Hellmuth (IKS); +Cc: Kevin, Scott Chacon, Andrew Ardill, git
In-Reply-To: <508FAE23.4080704@ira.uka.de>
I just checked and the issue seems to be fixed! Clicked around on a
bunch of previously broken links and they work!
On Tue, Oct 30, 2012 at 3:38 AM, Holger Hellmuth (IKS)
<hellmuth@ira.uka.de> wrote:
> Am 30.10.2012 09:07, schrieb Mike Norman:
>
>> Not seen any recently. I'm guessing the dev is in the path of
>> hurricane Sandy? (Not sarcasm, btw.)
>
>
> Do you still see failures? I checked out the website just now and it seemed
> to work flawlessly (at least the links I tried, could not find any Sharing
> or Updating section). New design since I last visited, more end-user polish.
>
>
^ permalink raw reply
* Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
From: Jonathan Nieder @ 2012-10-30 23:55 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Junio C Hamano, Sverre Rabbelier,
Johannes Schindelin, Elijah Newren
In-Reply-To: <CAMP44s3ArAQXH+-EbH4MHYaV6fTAWdwGzBdZwzn_qtCABHyonQ@mail.gmail.com>
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". 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.
I haven't reviewed the later patches in the series; maybe they fix
these things. 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, and the context one might use to convincingly explain
that a patch is not introducing a regression turns out to be essential
for many other purposes as well.
Jonathan
^ permalink raw reply
* Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
From: Jonathan Nieder @ 2012-10-31 0:11 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>
(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>
[...]
> --- 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:
(
cd localclone &&
git branch point-to-master master &&
git push origin point-to-master
) &&
(
cd server &&
git rev-parse --verify refs/heads/point-to-master
)
[...]
> +test_expect_success 'refs are updated even if no commits need to be exported' '
> + git fast-export --import-marks=tmp-marks \
> + --export-marks=tmp-marks master > /dev/null &&
The redirect just makes the test log with "-v" less informative, so
I'd drop it.
> + 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.
Hope that helps,
Jonathan
^ permalink raw reply
* 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
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