Git development
 help / color / mirror / Atom feed
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Junio C Hamano @ 2012-02-23 19:23 UTC (permalink / raw)
  To: kusmabite; +Cc: git
In-Reply-To: <CABPQNSZVOjOKpqv4s1ZCEQRd_yT3us3mqC9aN-KK5PHqztYQQg@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

>> +       /* Use editor if stdin and stdout are the same and is a tty */
>> +       return (!fstat(0, &st_stdin) &&
>> +               !fstat(1, &st_stdout) &&
>> +               isatty(0) &&
>> +               st_stdin.st_dev == st_stdout.st_dev &&
>> +               st_stdin.st_ino == st_stdout.st_ino &&
>> +               st_stdin.st_mode == st_stdout.st_mode);
>
> I just stumbled over this code, and I got a bit worried; the
> stat-implementation we use on Windows sets st_ino to 0, so
> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>
> Perhaps we should add a check for isatty(1) here as well? ...
> Or is there something I'm missing here?

No, the intention was not "we do this whether standard output is tty or
not", but was "we check that fd#0 and fd#1 are connected to the same
device by trusting stat() to do the right thing, so checking isatty(0)
is sufficient".  As that "trusting stat()" assumption does not hold for
your platform, we would need to add isatty(1) to accomodate it.

Thanks for a set of sharp eyes.

^ permalink raw reply

* Re: git log -z doesn't separate commits with NULs
From: Jeff King @ 2012-02-23 19:34 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: Jakub Narebski, git
In-Reply-To: <4F4643BB.8090001@gmail.com>

On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:

> For example there are commits that affect not only files in folder A
> but files in folder B, C and so on.  If I do format-patch that will
> give me nice patches, but there are modifications of folders B, C and
> so on there.  I do not know a way to generate patches via format-patch
> that affect only files in folder A.

Doesn't:

  git format-patch HEAD~3..HEAD SomePath

do what you want? It is certainly designed to, and it seems to work for
me.

-Peff

^ permalink raw reply

* Re: [PATCH] pretty: add '*' modifier to add LF after non-empty
From: Junio C Hamano @ 2012-02-23 19:53 UTC (permalink / raw)
  To: Luc Pionchon; +Cc: git
In-Reply-To: <1330002637-9347-1-git-send-email-pionchon.luc@gmail.com>

Luc Pionchon <pionchon.luc@gmail.com> writes:

> Add the '*' modifier, similar to the '+' modifier,
> to add a line-feed after a non-empty placeholder.

Hrm, I thought I designed the plus and minus fairly carefully so that
nobody needs to add this later.

Wouldn't it be sufficient to write

        Foo%n%-d

that says "We usually have LF after Foo, and write %d after that, but we
might not have anything interesting in %d at all, in which case we don't
add that %n"?

> +test_expect_success 'add LF after non-empty (1) (empty)' '
> +	git show -s --pretty=format:"%*d%s%nfoo%n" HEAD^^ >actual &&

Shouldn't this be equivalent to "%n%-d%s%nfoo%n", which in turn is covered
by one of the previous tests (del LF before empty)?

^ permalink raw reply

* Re: [PATCHv5] git-p4: add initial support for RCS keywords
From: Junio C Hamano @ 2012-02-23 20:03 UTC (permalink / raw)
  To: Luke Diamand; +Cc: git, Pete Wyckoff, Eric Scouten
In-Reply-To: <1329983490-4033-2-git-send-email-luke@diamand.org>

Hmm...  I somehow was expecting that the patch will be an incremental
update to what has already been queued in 'next' for the past several
days, with its commit log describing what the incremental improvements
are.

I'll revert it out and apply this on top.  Thanks.

^ permalink raw reply

* Re: git log -z doesn't separate commits with NULs
From: Junio C Hamano @ 2012-02-23 20:07 UTC (permalink / raw)
  To: Jeff King; +Cc: Nikolaj Shurkaev, Jakub Narebski, git
In-Reply-To: <20120223193451.GB30132@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
>
> Doesn't:
>
>   git format-patch HEAD~3..HEAD SomePath
>
> do what you want? It is certainly designed to, and it seems to work for
> me.

It is not quite "designed to", though.

It happens to work that way, and I do not think we want to forbid its use,
but we would want to discourage anybody from blindly using it without
thinking if the end results suits his/her purpose (and the reason should
be obvious to those who think, the hint is "log message").

^ permalink raw reply

* Re: measuring the % change between two commits
From: Junio C Hamano @ 2012-02-23 20:09 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gXQaE+RZxe4S7vqAyPka9N9PAoe+557FaLO+JJoJFshA@mail.gmail.com>

Sitaram Chamarty <sitaramc@gmail.com> writes:

> I could do a --numstat and then do a 'wc -l' on each file I guess, but
> I was hoping to avoid that.

Either you do it or the git core does it, but I am not sure if the use
case is common enough to warrant additional code on the core side.

^ permalink raw reply

* Re: measuring the % change between two commits
From: Junio C Hamano @ 2012-02-23 20:13 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gXQaE+RZxe4S7vqAyPka9N9PAoe+557FaLO+JJoJFshA@mail.gmail.com>

Sitaram Chamarty <sitaramc@gmail.com> writes:

> I could do a --numstat and then do a 'wc -l' on each file I guess, but
> I was hoping to avoid that.
>
> --dirstat gives you a percentage but does not count the top level directory.

Note that dirstat is not about "how much damage was caused to the entire
codebase".  It only measures "How is the damage this patch causes
distributed across directories it touches".  It was unclear from your "a %
measure for the changes between two commits" which one you meant, but I am
guessing from your "--numstat and wc -l" reference that you are asking for
the former, e.g. we have 300,000 lines of code and between these two
commits 10,000 lines changed, hence we updated 3% of the codebase during
that period".

^ permalink raw reply

* [PATCH] mingw: work around stat-limitation
From: Erik Faye-Lund @ 2012-02-23 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, j6t, msysgit

Our stat implementation for Windows always sets st_ino to 0. This
means that checking if isatty(0) and comparing the reported inodes
of stdout and stdin is not sufficient to detect that both are
pointing to the same TTY.

Luckily, there's only one console on Windows, so adding a check for
isatty(1) should do the trick. For platforms where inodes are
reported correctly, this should still be correct.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

Here's a proper patch for this glitch.

 builtin/merge.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index ed0f959..bef01e3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1130,6 +1130,7 @@ static int default_edit_option(void)
 	return (!fstat(0, &st_stdin) &&
 		!fstat(1, &st_stdout) &&
 		isatty(0) &&
+		isatty(1) &&
 		st_stdin.st_dev == st_stdout.st_dev &&
 		st_stdin.st_ino == st_stdout.st_ino &&
 		st_stdin.st_mode == st_stdout.st_mode);
-- 
1.7.9

^ permalink raw reply related

* Re: [PATCH] merge: use editor by default in interactive sessions
From: Junio C Hamano @ 2012-02-23 20:26 UTC (permalink / raw)
  To: kusmabite; +Cc: git
In-Reply-To: <7vd3954ame.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> +       /* Use editor if stdin and stdout are the same and is a tty */
>>> +       return (!fstat(0, &st_stdin) &&
>>> +               !fstat(1, &st_stdout) &&
>>> +               isatty(0) &&
>>> +               st_stdin.st_dev == st_stdout.st_dev &&
>>> +               st_stdin.st_ino == st_stdout.st_ino &&
>>> +               st_stdin.st_mode == st_stdout.st_mode);
>>
>> I just stumbled over this code, and I got a bit worried; the
>> stat-implementation we use on Windows sets st_ino to 0, so
>> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>>
>> Perhaps we should add a check for isatty(1) here as well? ...
>> Or is there something I'm missing here?
>
> No, the intention was ...

s/No,/No, you are not missing anything./;

I'll queue it with this explanation:

    merge: do not trust fstat(2) too much when checking interactiveness
    
    The heuristic used by "git merge" to decide if it automatically gives an
    editor upon clean automerge is to see if the standard input and the
    standard output is the same device and is a tty, we are in an interactive
    session.  "The same device" test was done by comparing fstat(2) result on
    the two file descriptors (and they must match), and we asked isatty() only
    for the standard input (we insist that they are the same device and there
    is no point asking tty-ness of the standard output).
    
    The stat(2) emulation on Windows port however does not give a usable value
    in st_ino field, so even if the standard output is connected to something
    different from the standard input, "The same device" test may incorrectly
    return true. To accomodate it, add another isatty() check for the standard
    output stream as well.
    
    Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks.

^ permalink raw reply

* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Junio C Hamano @ 2012-02-23 20:28 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1330013115-26355-1-git-send-email-cmn@elego.de>

Carlos Martín Nieto <cmn@elego.de> writes:

> The current message is too long and at too low a level for anybody to
> understand it if they don't know about the configuration format
> already.
>
> Reformat it to show the commands a user would be expected to use,
> instead of the contents of the configuration file.
> ---

Sounds like a change going in the right direction.  I am unsure if it is a
good idea to remove "See git-config...", but otherwise I like the updated
text much better.

But of course I am not the target audience, so let's see what we hear from
others.

Thanks.

^ permalink raw reply

* Re: [PATCH] merge: use editor by default in interactive sessions
From: Erik Faye-Lund @ 2012-02-23 20:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlint2t5h.fsf@alter.siamese.dyndns.org>

On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>>> +       /* Use editor if stdin and stdout are the same and is a tty */
>>>> +       return (!fstat(0, &st_stdin) &&
>>>> +               !fstat(1, &st_stdout) &&
>>>> +               isatty(0) &&
>>>> +               st_stdin.st_dev == st_stdout.st_dev &&
>>>> +               st_stdin.st_ino == st_stdout.st_ino &&
>>>> +               st_stdin.st_mode == st_stdout.st_mode);
>>>
>>> I just stumbled over this code, and I got a bit worried; the
>>> stat-implementation we use on Windows sets st_ino to 0, so
>>> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>>>
>>> Perhaps we should add a check for isatty(1) here as well? ...
>>> Or is there something I'm missing here?
>>
>> No, the intention was ...
>
> s/No,/No, you are not missing anything./;
>
> I'll queue it with this explanation:
>
>    merge: do not trust fstat(2) too much when checking interactiveness
>
>    The heuristic used by "git merge" to decide if it automatically gives an
>    editor upon clean automerge is to see if the standard input and the
>    standard output is the same device and is a tty, we are in an interactive
>    session.  "The same device" test was done by comparing fstat(2) result on
>    the two file descriptors (and they must match), and we asked isatty() only
>    for the standard input (we insist that they are the same device and there
>    is no point asking tty-ness of the standard output).
>
>    The stat(2) emulation on Windows port however does not give a usable value
>    in st_ino field, so even if the standard output is connected to something

Shouldn't that be "emulation _in the_ Windows port" and "in _the_ st_ino field"?

>    different from the standard input, "The same device" test may incorrectly
>    return true. To accomodate it, add another isatty() check for the standard
>    output stream as well.
>
>    Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks.

I just sent a mail with a proper-ish commit message, but I like yours
better as it explains the symptom a bit.

^ permalink raw reply

* Re: [PATCHv3 0/3] gitweb: Faster project search
From: Junio C Hamano @ 2012-02-23 20:54 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <1330011779-7803-1-git-send-email-jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> [Cc-ing Junio because of his involvement in discussion]

Heh, drop that line. People can guess that from the context.

> These patches are separated from first part of previous version of
> this series
>
>   "[PATCHv2 0/8] gitweb: Faster and improved project search"
>   http://thread.gmane.org/gmane.comp.version-control.git/190852
>
> It is meant to replace 'jn/gitweb-search-optim' in pu

I think the result is a lot easier than the previous rounds; will replace
the said topic and move it to 'next' soonish.

Thanks.

^ permalink raw reply

* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Johannes Schindelin @ 2012-02-23 21:26 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <1330028744-5280-1-git-send-email-kusmabite@gmail.com>

Hi,

On Thu, 23 Feb 2012, Erik Faye-Lund wrote:

> Our stat implementation for Windows always sets st_ino to 0. This
> means that checking if isatty(0) and comparing the reported inodes
> of stdout and stdin is not sufficient to detect that both are
> pointing to the same TTY.
> 
> Luckily, there's only one console on Windows, so adding a check for
> isatty(1) should do the trick. For platforms where inodes are
> reported correctly, this should still be correct.

Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
not? Should they not access the very same console object?

Thanks,
Dscho

^ permalink raw reply

* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Erik Faye-Lund @ 2012-02-23 21:36 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <alpine.DEB.1.00.1202231525320.3340@s15462909.onlinehome-server.info>

On Thu, Feb 23, 2012 at 10:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
>
>> Our stat implementation for Windows always sets st_ino to 0. This
>> means that checking if isatty(0) and comparing the reported inodes
>> of stdout and stdin is not sufficient to detect that both are
>> pointing to the same TTY.
>>
>> Luckily, there's only one console on Windows, so adding a check for
>> isatty(1) should do the trick. For platforms where inodes are
>> reported correctly, this should still be correct.
>
> Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
> not? Should they not access the very same console object?
>

The point is that they might not. The old test would draw the wrong
conclusion if isatty(0) was true, but isatty(1) was not because it
used st_ino to verify that stdin and stdout pointed to the same
terminal. The reason it did that was to catch cases where stdin and
stdout pointed to different terminals, AFAICT. Not checking isatty(1)
was simply an optimization, which works when st_ino is filled out
correctly.

On Windows there is only one terminal, so it's sufficient to check if
both are connected.

^ permalink raw reply

* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Johannes Schindelin @ 2012-02-23 22:37 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <CABPQNSZ+=no5PKkaa8_44FbOzh_W4A5knoxKgDQ2wTvOaeKdew@mail.gmail.com>

Hi kusma,

On Thu, 23 Feb 2012, Erik Faye-Lund wrote:

> On Thu, Feb 23, 2012 at 10:26 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
> >
> >> Our stat implementation for Windows always sets st_ino to 0. This
> >> means that checking if isatty(0) and comparing the reported inodes
> >> of stdout and stdin is not sufficient to detect that both are
> >> pointing to the same TTY.
> >>
> >> Luckily, there's only one console on Windows, so adding a check for
> >> isatty(1) should do the trick. For platforms where inodes are
> >> reported correctly, this should still be correct.
> >
> > Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
> > not? Should they not access the very same console object?
> >
> 
> The point is that they might not. The old test would draw the wrong
> conclusion if isatty(0) was true, but isatty(1) was not because it
> used st_ino to verify that stdin and stdout pointed to the same
> terminal. The reason it did that was to catch cases where stdin and
> stdout pointed to different terminals, AFAICT. Not checking isatty(1)
> was simply an optimization, which works when st_ino is filled out
> correctly.
> 
> On Windows there is only one terminal, so it's sufficient to check if
> both are connected.

Ah, okay. Thanks for answering so patiently!

ACK.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Steven Walter @ 2012-02-23 23:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: normalperson, git
In-Reply-To: <7vmx8bcv4u.fsf@alter.siamese.dyndns.org>

Signed-Off-By: Steven Walter <stevenrwalter@gmail.com>

On Wed, Feb 22, 2012 at 12:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Steven Walter <stevenrwalter@gmail.com> writes:
>
>>>> +     test -e "$SVN_TREE"/bar/zzz/yyy ' || true
>>>
>>> Care to explain what this " || true" is doing here, please?
>>
>> Ahh, good catch.  I think the answer is that it shouldn't be there.
>> It was originally there because of the "test_must_fail" line, I think
>> (at least the other tests that use test_must_fail also have "||
>> true").
>
> Ok, that may explain the copy&paste error.
>
> But I do not think test_must_fail followed by || true makes much sense,
> either.  The purpose of "test_must_fail" is to make sure the tested git
> command exits with non-zero status in a controlled way (i.e. not crash)
> so if the tested command that is expected to exit with non-zero status
> exited with zero status, the test has detected an *error*.  E.g. if you
> know that the index and the working tree are different at one point in the
> test sequence, you would say:
>
>        ... other setup steps ... &&
>        test_must_fail git diff --exit-code &&
>        ... and other tests ...
>
> so that failure by "git diff --exit-code" to exit with non-zero status
> (i.e. it did not find any difference when it should have) breaks the &&
> cascade.
>
> I just took a quick look at t9100 but I think all " || true" can be safely
> removed.  None of them is associated with test_must_fail in any way.  For
> whatever reason, these test seem to do
>
>        test_expect_success 'label of the test' '
>                body of the test
>        ' || true
>
> for no good reason.
>
>> Do you want to just fix that up, or a new version of the original patch,
>> or a fix on top of the original patches?
>
> Eric queued the patch and then had me pull it as part of his history
> already, so it is doubly too late to replace it.
>
> Can you apply this patch and re-test?
>
>
>  t/t9100-git-svn-basic.sh |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
> index 4029f84..749b75e 100755
> --- a/t/t9100-git-svn-basic.sh
> +++ b/t/t9100-git-svn-basic.sh
> @@ -65,7 +65,8 @@ test_expect_success "$name" "
>        git update-index --add dir/file/file &&
>        git commit -m '$name' &&
>        test_must_fail git svn set-tree --find-copies-harder --rmdir \
> -               ${remotes_git_svn}..mybranch" || true
> +               ${remotes_git_svn}..mybranch
> +"
>
>
>  name='detect node change from directory to file #1'
> @@ -79,7 +80,8 @@ test_expect_success "$name" '
>        git update-index --add -- bar &&
>        git commit -m "$name" &&
>        test_must_fail git svn set-tree --find-copies-harder --rmdir \
> -               ${remotes_git_svn}..mybranch2' || true
> +               ${remotes_git_svn}..mybranch2
> +'
>
>
>  name='detect node change from file to directory #2'
> @@ -96,7 +98,8 @@ test_expect_success "$name" '
>                ${remotes_git_svn}..mybranch3 &&
>        svn_cmd up "$SVN_TREE" &&
>        test -d "$SVN_TREE"/bar/zzz &&
> -       test -e "$SVN_TREE"/bar/zzz/yyy ' || true
> +       test -e "$SVN_TREE"/bar/zzz/yyy
> +'
>
>  name='detect node change from directory to file #2'
>  test_expect_success "$name" '
> @@ -109,7 +112,8 @@ test_expect_success "$name" '
>        git update-index --add -- dir &&
>        git commit -m "$name" &&
>        test_must_fail git svn set-tree --find-copies-harder --rmdir \
> -               ${remotes_git_svn}..mybranch4' || true
> +               ${remotes_git_svn}..mybranch4
> +'
>
>
>  name='remove executable bit from a file'
> @@ -162,7 +166,7 @@ test_expect_success "$name" '
>
>  name='modify a symlink to become a file'
>  test_expect_success "$name" '
> -       echo git help > help || true &&
> +       echo git help >help &&
>        rm exec-2.sh &&
>        cp help exec-2.sh &&
>        git update-index exec-2.sh &&



-- 
-Steven Walter <stevenrwalter@gmail.com>

^ permalink raw reply

* Re: [PATCH v2 3/4] t5704: match tests to modern style
From: Junio C Hamano @ 2012-02-23 23:36 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Johannes Sixt, Jeff King, Jannis Pohlmann, git
In-Reply-To: <c6579adc648119fbd1c54e2a1a9a0de86e0e8b57.1329988335.git.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> Fix all of them.  There's a catch to the last point: test_commit
> creates a tag.  We still change it to test_commit, and explicitly
> delete the tags, so as to highlight that the test relies on not having
> them.

I do not have an objection to the use of these three test_commits, and I
do not have an objection to delete the extra three tags they create so
that the set of refs in the repository matches what the later test
expects, either.

But I found the explanation a bit iffy.

>  test_expect_success 'setup' '
> +	test_commit initial &&
>  	test_tick &&
>  	git tag -m tag tag &&
> +	test_commit second &&
> +	test_commit third &&
> +	git tag -d initial &&
> +	git tag -d second &&
> +	git tag -d third
>  '
>  
>  test_expect_success 'tags can be excluded by rev-list options' '
>  	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
>  	git ls-remote bundle > output &&
>  	! grep tag output
>  '

If you do not delete 'third' tag, which matches the tip of the current
branch, the resulting bundle created with "create --all" would end up
containing that tag, and you will see it in the ls-remote output.

But the funny thing is that you can leave initial and second in the
repository and the resulting bundle still passes the test.  'initial',
'second' and 'tag' are excluded.  Exclusion of 'tag' tag (sheesh, it makes
this conversation more confusing than necessary) is what this test checks,
and the date range given to the rev-list ensures that initial and second
are not included.

But the tip of the current branch and the lightweight 'third' tag both
point at the same commit object, and that, together with the fact that we
ask for '--all', is the reason why it is included in the result, making
the test fail.

So perhaps a better fix may be to do something like the attached on top,
and rewrite everything after "Fix all of them".

	... Fix all of them.

        Also rename the manually created tag 'tag' that points at a commit
        that is older than the --since threshold a later test uses, to a
        more descriptive 'ancienttag', and update the check that reads
        from the resulting bundle with ls-remote to look for 'ancienttag'.
        The purpose of this test is to make sure that the tag that
        predates the date range is not in the resulting bundle, but
        because these test_commit also will create tags, one of which
        (namely, 'third') points at a commit that is newer than that
        threshold, ls-remote will list it in its output. Looking for 'tag'
        will match refs/tags/third, making the test incorrectly fail.

diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index a51c8b0..3c436e7 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -6,18 +6,15 @@ test_description='some bundle related tests'
 test_expect_success 'setup' '
 	test_commit initial &&
 	test_tick &&
-	git tag -m tag tag &&
+	git tag -m tag antienttag &&
 	test_commit second &&
-	test_commit third &&
-	git tag -d initial &&
-	git tag -d second &&
-	git tag -d third
+	test_commit third
 '
 
 test_expect_success 'tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
 	git ls-remote bundle > output &&
-	! grep tag output
+	! grep antienttag output
 '
 
 test_expect_success 'die if bundle file cannot be created' '

^ permalink raw reply related

* git gui:  how to fetch a single branch
From: Matt Seitz (matseitz) @ 2012-02-23 23:58 UTC (permalink / raw)
  To: git

How can I use "git gui" to fetch a single branch from a remote
repository?

If I select Remote->Fetch From, and then a remote repository, it appears
to always fetch all the branches from the remote repository with no
option to select which branch I want to fetch.

^ permalink raw reply

* [PATCH v3] contrib: added git-diffall
From: Tim Henigan @ 2012-02-24  0:49 UTC (permalink / raw)
  To: git, gitster; +Cc: davvid, stefano.lattarini, tim.henigan

The 'git difftool' command allows the user to view diffs using an
external tool.  It runs a separate instance of the tool for each
file in the diff. This makes it tedious to review changes spanning
multiple files.

The 'git-diffall' script instead prepares temporary directories
with the files to be compared and launches a single instance of
the external diff tool to view them (i.e. a directory diff).

The 'diff.tool' or 'merge.tool' configuration variable is used
to specify which external tool is used.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This script has been hosted on GitHub [1] since April 2010. Enough people
have found it useful that I hope it will be considered for inclusion in
the standard git install, either in contrib or as a new core command.

Changes in v3:
    - Fixed a bug that caused failures if file names included spaces
    - Added unique suffix to tmp dir name (tmp/git-diffall-tmp.$$)
    - Renamed "common_ancestor" to "merge_base"
    - Cleaned up README to be more accurate
    - Added useful error message if --extcmd is final option, but no
      command was specified
    - Removed spaces after redirection operators

    v3 matches commit f36e4881e5 on GitHub [1].

Changes in v2:
    - Changed to #!/bin/sh
    - Eliminated use of 'which' statements
    - Fixed trap function to actually run on abnormal exit
    - Simplified path concatenation logic ($IFS)
    - Corrected indentation errors
    - Improved readability of while loop
    - Cleaned up quoting of variables

    v2 matches commit 5d4b90de3 on GitHub [1].

[1]: https://github.com/thenigan/git-diffall

 contrib/diffall/README      |   24 ++++
 contrib/diffall/git-diffall |  261 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 285 insertions(+)
 create mode 100644 contrib/diffall/README
 create mode 100755 contrib/diffall/git-diffall

diff --git a/contrib/diffall/README b/contrib/diffall/README
new file mode 100644
index 0000000..111f3f6
--- /dev/null
+++ b/contrib/diffall/README
@@ -0,0 +1,24 @@
+The git-diffall script provides a directory based diff mechanism
+for git.  The script relies on the diff.tool configuration option
+to determine what diff viewer is used.
+
+This script is compatible with most common forms used to specify a
+range of revisions to diff:
+
+  1. git diffall: shows diff between working tree and staged changes
+  2. git diffall --cached [<commit>]: shows diff between staged
+     changes and HEAD (or other named commit)
+  3. git diffall <commit>: shows diff between working tree and named
+     commit
+  4. git diffall <commit> <commit>: show diff between two named commit
+  5. git diffall <commit>..<commit>: same as above
+  6. git diffall <commit>...<commit>: show the changes on the branch
+     containing and up to the second , starting at a common ancestor
+     of both <commit>
+
+Note: all forms take an optional path limiter [--] [<path>*]
+
+This script is based on an example provided by Thomas Rast on the
+Git list [1]:
+
+[1] http://thread.gmane.org/gmane.comp.version-control.git/124807
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
new file mode 100755
index 0000000..e00fe89
--- /dev/null
+++ b/contrib/diffall/git-diffall
@@ -0,0 +1,261 @@
+#!/bin/sh
+# Copyright 2010 - 2012, Tim Henigan <tim.henigan@gmail.com>
+#
+# Perform a directory diff between commits in the repository using
+# the external diff or merge tool specified in the user's config.
+
+USAGE='[--cached] [--copy-back] [-x|--extcmd=<command>] <commit>{0,2} -- <path>*
+
+    --cached     Compare to the index rather than the working tree.
+
+    --copy-back  Copy files back to the working tree when the diff
+                 tool exits (in case they were modified by the
+                 user).  This option is only valid if the diff
+                 compared with the working tree.
+
+    -x=<command>
+    --extcmd=<command>  Specify a custom command for viewing diffs.
+                 git-diffall ignores the configured defaults and
+                 runs $command $LOCAL $REMOTE when this option is
+                 specified. Additionally, $BASE is set in the
+                 environment.
+'
+
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+TOOL_MODE=diff
+. "$(git --exec-path)/git-mergetool--lib"
+
+merge_tool="$(get_merge_tool)"
+if test -z "$merge_tool"
+then
+	echo "Error: Either the 'diff.tool' or 'merge.tool' option must be set."
+	usage
+fi
+
+start_dir=$(pwd)
+
+# needed to access tar utility
+cdup=$(git rev-parse --show-cdup) &&
+cd "$cdup" || {
+	echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+	exit 1
+}
+
+# mktemp is not available on all platforms (missing from msysgit)
+# Use a hard-coded tmp dir if it is not available
+tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
+	tmp=/git-diffall-tmp.$$
+	mkdir "$tmp" || exit 1
+}
+
+trap 'rm -rf "$tmp" 2>/dev/null' EXIT
+
+left=
+right=
+paths=
+path_sep=
+compare_staged=
+merge_base=
+left_dir=
+right_dir=
+diff_tool=
+copy_back=
+
+while test $# != 0
+do
+	case "$1" in
+	-h|--h|--he|--hel|--help)
+		usage
+		;;
+	--cached)
+		compare_staged=1
+		;;
+	--copy-back)
+		copy_back=1
+		;;
+	-x|--e|--ex|--ext|--extc|--extcm|--extcmd)
+		if test -z $2
+		then
+			echo You must specify the tool for use with --extcmd
+			usage
+		else
+			diff_tool=$2
+			shift
+		fi
+		;;
+	--)
+		path_sep=1
+		;;
+	-*)
+		echo Invalid option: "$1"
+		usage
+		;;
+	*)
+		# could be commit, commit range or path limiter
+		case "$1" in
+		*...*)
+			left=${1%...*}
+			right=${1#*...}
+			merge_base=1
+			;;
+		*..*)
+			left=${1%..*}
+			right=${1#*..}
+			;;
+		*)
+			if test -n "$path_sep"
+			then
+				paths="$paths$1 "
+			elif test -z "$left"
+			then
+				left=$1
+			elif test -z "$right"
+			then
+				right=$1
+			else
+				paths="$paths$1 "
+			fi
+			;;
+		esac
+		;;
+	esac
+	shift
+done
+
+# Determine the set of files which changed
+if test -n "$left" && test -n "$right"
+then
+	left_dir="cmt-$(git rev-parse --short $left)"
+	right_dir="cmt-$(git rev-parse --short $right)"
+
+	if test -n "$compare_staged"
+	then
+		usage
+	elif test -n "$merge_base"
+	then
+		git diff --name-only "$left"..."$right" -- $paths >"$tmp/filelist"
+	else
+		git diff --name-only "$left" "$right" -- $paths >"$tmp/filelist"
+	fi
+elif test -n "$left"
+then
+	left_dir="cmt-$(git rev-parse --short $left)"
+
+	if test -n "$compare_staged"
+	then
+		right_dir="staged"
+		git diff --name-only --cached "$left" -- $paths >"$tmp/filelist"
+	else
+		right_dir="working_tree"
+		git diff --name-only "$left" -- $paths >"$tmp/filelist"
+	fi
+else
+	left_dir="HEAD"
+
+	if test -n "$compare_staged"
+	then
+		right_dir="staged"
+		git diff --name-only --cached -- $paths >"$tmp/filelist"
+	else
+		right_dir="working_tree"
+		git diff --name-only -- $paths >"$tmp/filelist"
+	fi
+fi
+
+# Exit immediately if there are no diffs
+if test ! -s "$tmp/filelist"
+then
+	exit 0
+fi
+
+if test -n "$copy_back" && test "$right_dir" != "working_tree"
+then
+	echo "--copy-back is only valid when diff includes the working tree."
+	exit 1
+fi
+
+# Create the named tmp directories that will hold the files to be compared
+mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
+
+# Populate the tmp/right_dir directory with the files to be compared
+if test -n "$right"
+then
+	while read name
+	do
+		ls_list=$(git ls-tree $right "$name")
+		if test -n "$ls_list"
+		then
+			mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+			git show "$right":"$name" >"$tmp/$right_dir/$name" || true
+		fi
+	done < "$tmp/filelist"
+elif test -n "$compare_staged"
+then
+	while read name
+	do
+		ls_list=$(git ls-files -- "$name")
+		if test -n "$ls_list"
+		then
+			mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+			git show :"$name" >"$tmp/$right_dir/$name"
+		fi
+	done < "$tmp/filelist"
+else
+	# Mac users have gnutar rather than tar
+	(tar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && tar -x)) || {
+		gnutar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && gnutar -x)
+	}
+fi
+
+# Populate the tmp/left_dir directory with the files to be compared
+while read name
+do
+	if test -n "$left"
+	then
+		ls_list=$(git ls-tree $left "$name")
+		if test -n "$ls_list"
+		then
+			mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+			git show "$left":"$name" >"$tmp/$left_dir/$name" || true
+		fi
+	else
+		if test -n "$compare_staged"
+		then
+			ls_list=$(git ls-tree HEAD "$name")
+			if test -n "$ls_list"
+			then
+				mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+				git show HEAD:"$name" >"$tmp/$left_dir/$name"
+			fi
+		else
+			mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+			git show :"$name" >"$tmp/$left_dir/$name"
+		fi
+	fi
+done < "$tmp/filelist"
+
+cd "$tmp"
+LOCAL="$left_dir"
+REMOTE="$right_dir"
+
+if test -n "$diff_tool"
+then
+	export BASE
+	eval $diff_tool '"$LOCAL"' '"$REMOTE"'
+else
+	run_merge_tool "$merge_tool" false
+fi
+
+# Copy files back to the working dir, if requested
+if test -n "$copy_back" && test "$right_dir" = "working_tree"
+then
+	cd "$start_dir"
+	git_top_dir=$(git rev-parse --show-toplevel)
+	find "$tmp/$right_dir" -type f |
+	while read file
+	do
+		cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
+	done
+fi
-- 
1.7.9.GIT

^ permalink raw reply related

* Re: git-subtree Ready #2
From: Avery Pennarun @ 2012-02-24  1:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, David A. Greene, git
In-Reply-To: <7vd399jdwc.fsf@alter.siamese.dyndns.org>

On Mon, Feb 20, 2012 at 6:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> It sounds like the simplest and cleanest would be to treat it as if its
> current version came as a patch submission, cook it just like any other
> topic in 'pu' down to 'next' down to eventually 'master', with the usual
> review cycle of pointing out what is wrong and needs fixing followed by a
> series of re-rolls.

Yeah, my original intent with git-subtree was to one day submit it as
basically a single patch against git.  That's why I have some slightly
suspicious commit messages in there (though in my defense, I think
*most* of the commit messages are quite sensible :)).

> After looking at the history of subtree branch there, however, I agree
> that it would not help anybody to have its history in my tree with log
> messages like these (excerpt from shortlog output):
> [...]
>      Docs: when pushing to github, the repo path needs to end in .git
> [...]

That commit message in particular I thought was perfectly fine; it's
specifically a fix to the git-subtree docs to clarify a question from
an actual user.

Overall I agree that there's little benefit in preserving the history,
at least as far as I can see, *except* that some code changes were
submitted by people other than me and squashing those changes might
conceivably cause licensing confusion down the road.  It's probably a
fairly quick exercise with git-filter-branch to get rid of the more
egregious commit message problems, if that's what we want to do.  (In
particular, just expurgating the entire 'todo' file from the history
probably makes plenty of sense.)

There's no value I can see in being able to do future merges from
outside the tree, so a filter-branch or rebase before merging should
be pretty harmless.

> The total amount of change does not look too bad, either:
>
>    $ git diff --stat master...origin/subtree
>     contrib/subtree/.gitignore         |    5 +
>     contrib/subtree/COPYING            |  339 +++++++++++++++++
>     contrib/subtree/Makefile           |   45 +++
>     contrib/subtree/README             |    8 +
>     contrib/subtree/git-subtree.sh     |  712 ++++++++++++++++++++++++++++++++++++
>     contrib/subtree/git-subtree.txt    |  366 ++++++++++++++++++
>     contrib/subtree/t/Makefile         |   71 ++++
>     contrib/subtree/t/t7900-subtree.sh |  508 +++++++++++++++++++++++++
>     contrib/subtree/todo               |   50 +++
>     t/test-lib.sh                      |   11 +-
>     10 files changed, 2114 insertions(+), 1 deletion(-)

Note that COPYING, .gitignore, Makefile, t/Makefile, todo, and README
should probably be ditched if it weren't going into contrib.  The
interesting files are git-subtree.{sh,txt} and t7900-subtree.sh.

> I haven't looked at the script fully, but it has an issue
> from its first line, which is marked with "#!/bin/bash".  It is unclear if
> it is infested by bash-isms beyond repair (in which case "#!/bin/bash" is
> fine), or it was written portably but was marked with "#!/bin/bash" just
> by inertia.

I'm generally pretty careful to avoid bashisms, but since my /bin/sh
is bash, I usually mark scripts with /bin/bash just to be safe until
someone has actually verified them with a non-bash shell.  I expect
few if any problems with that part.

> A patch that corresponds to the above diffstat immediately
> shows many style issues including trailing eye-sore whitespaces.
>
> It seems that it is even capable of installing from contrib/subtree, so
> keeping it in contrib/ while many issues it may have gets fixed would not
> hurt the original goal of giving the script more visibility.

Personally, I would prefer to just iterate the patch a few times to
correct the coding style problems you see, rather than merging into
contrib where it might be forgotten rather than fixed.  As Peff
alluded, people who want to install it separately from git already
can; if we're going to merge it into git, let's do it right.

Have fun,

AVery

^ permalink raw reply

* [PATCH] index-pack: remove real_type from struct object_entry
From: Nguyễn Thái Ngọc Duy @ 2012-02-24  2:42 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

Since ce3f6dc (fix multiple issues in index-pack), real_type is
identical to type, there's no reason to keep it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I'm pretty sure I read it right (i.e. type == real_type), but I may
 have overlooked something. Not so sure why real_type was introduced
 in the first place..

 builtin/index-pack.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dd1c5c9..2db9a35 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -18,7 +18,6 @@ struct object_entry {
 	unsigned long size;
 	unsigned int hdr_size;
 	enum object_type type;
-	enum object_type real_type;
 	unsigned delta_depth;
 	int base_object_no;
 };
@@ -581,7 +580,6 @@ static void resolve_delta(struct object_entry *delta_obj,
 {
 	void *base_data, *delta_data;
 
-	delta_obj->real_type = base->obj->real_type;
 	delta_obj->delta_depth = base->obj->delta_depth + 1;
 	if (deepest_delta < delta_obj->delta_depth)
 		deepest_delta = delta_obj->delta_depth;
@@ -594,7 +592,7 @@ static void resolve_delta(struct object_entry *delta_obj,
 	free(delta_data);
 	if (!result->data)
 		bad_object(delta_obj->idx.offset, "failed to apply delta");
-	sha1_object(result->data, result->size, delta_obj->real_type,
+	sha1_object(result->data, result->size, delta_obj->type,
 		    delta_obj->idx.sha1);
 	nr_resolved_deltas++;
 }
@@ -626,7 +624,7 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,
 		struct object_entry *child = objects + deltas[base->ref_first].obj_no;
 		struct base_data *result = alloc_base_data();
 
-		assert(child->real_type == OBJ_REF_DELTA);
+		assert(child->type == OBJ_REF_DELTA);
 		resolve_delta(child, base, result);
 		if (base->ref_first == base->ref_last && base->ofs_last == -1)
 			free_base_data(base);
@@ -639,7 +637,7 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,
 		struct object_entry *child = objects + deltas[base->ofs_first].obj_no;
 		struct base_data *result = alloc_base_data();
 
-		assert(child->real_type == OBJ_OFS_DELTA);
+		assert(child->type == OBJ_OFS_DELTA);
 		resolve_delta(child, base, result);
 		if (base->ofs_first == base->ofs_last)
 			free_base_data(base);
@@ -702,7 +700,6 @@ static void parse_pack_objects(unsigned char *sha1)
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
 		void *data = unpack_raw_entry(obj, &delta->base);
-		obj->real_type = obj->type;
 		if (is_delta_type(obj->type)) {
 			nr_deltas++;
 			delta->obj_no = i;
@@ -805,7 +802,6 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,
 	obj[0].size = size;
 	obj[0].hdr_size = n;
 	obj[0].type = type;
-	obj[0].real_type = type;
 	obj[1].idx.offset = obj[0].idx.offset + n;
 	obj[1].idx.offset += write_compressed(f, buf, size);
 	obj[0].idx.crc32 = crc32_end(f);
@@ -838,7 +834,7 @@ static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
 	 */
 	sorted_by_pos = xmalloc(nr_unresolved * sizeof(*sorted_by_pos));
 	for (i = 0; i < nr_deltas; i++) {
-		if (objects[deltas[i].obj_no].real_type != OBJ_REF_DELTA)
+		if (objects[deltas[i].obj_no].type != OBJ_REF_DELTA)
 			continue;
 		sorted_by_pos[n++] = &deltas[i];
 	}
@@ -849,7 +845,7 @@ static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
 		enum object_type type;
 		struct base_data *base_obj = alloc_base_data();
 
-		if (objects[d->obj_no].real_type != OBJ_REF_DELTA)
+		if (objects[d->obj_no].type != OBJ_REF_DELTA)
 			continue;
 		base_obj->data = read_sha1_file(d->base.sha1, &type, &base_obj->size);
 		if (!base_obj->data)
@@ -1053,7 +1049,7 @@ static void show_pack_info(int stat_only)
 			continue;
 		printf("%s %-6s %lu %lu %"PRIuMAX,
 		       sha1_to_hex(obj->idx.sha1),
-		       typename(obj->real_type), obj->size,
+		       typename(obj->type), obj->size,
 		       (unsigned long)(obj[1].idx.offset - obj->idx.offset),
 		       (uintmax_t)obj->idx.offset);
 		if (is_delta_type(obj->type)) {
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* [BUG?] bulk checkin does not respect filters
From: Jeff King @ 2012-02-24  3:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

If I do this:

  git init repo &&
  cd repo &&
  echo foo >small &&
  cat small small small small >large &&
  echo '* filter=foo2bar' >.gitattributes &&
  git config filter.foo2bar.clean 'sed s/foo/bar/' &&
  git config core.bigfilethreshold 10 &&
  git add . &&
  echo "===> small" && git cat-file blob :small
  echo "===> large" && git cat-file blob :large

the output I get is:

  ===> small
  bar
  ===> large
  foo
  foo
  foo
  foo

I.e., the clean filter is not applied to the bulk checkin file. Nor can
it be easily, because we need to know the size of the file to write the
blob header, and we don't know that until we see all of the filter's
output.

In practice, I don't know if this is a huge deal, as people aren't going
to be asking to de-CRLF files that actually cross the 512M
bigfilethreshold (OTOH, I seem to recall there are some filters floating
around for normalizing gzip'd files, which could plausibly be gigantic).

But it seems like the right choice when we see this conflict is not
"don't do filters for streaming checkin", but rather "don't do streaming
checkin when filters are in use" (because streaming is an optimization,
and filters are about correctness).

It would be even nicer if filters could play well with bulk checkin, but
I think that would involve streaming to a tempfile, checking the size of
the file, and then streaming that into an object. Which is better than
putting the whole thing in memory if it would involve swapping, but
probably worse than doing so if you can possibly fit the whole thing in
(because you're doing a ton of extra I/O for the tempfile).

Thoughts? Was this intentional, or just overlooked?

-Peff

^ permalink raw reply

* Re: [BUG?] bulk checkin does not respect filters
From: Junio C Hamano @ 2012-02-24  3:17 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20120224030244.GA15742@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Thoughts? Was this intentional, or just overlooked?

This is intentional in the sense it is not worth worrying about (I
personally consider large and binary gunk equivalent and something nobody
should care about anything more than 1. what the exact contents it had at
each point in history and 2. if it did or did not change between two
versions, but definitely not 3. how different these two versions were),
and does not deserve the complexity to support filtering and textconv'ing.

If somebody strongly feels about lifting the limitation with a clean patch
that does not harm the common case codepaths, patches are welcome, but I
am not planning to do it myself ;-)

It is worth documenting this limitation, though.

^ permalink raw reply

* Re: [BUG?] bulk checkin does not respect filters
From: Junio C Hamano @ 2012-02-24  3:42 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <7vvcmw2a3m.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> Thoughts? Was this intentional, or just overlooked?

It is a bit curious that anything filtered even goes to the streaming
codepath, given this piece of code in write_entry() in entry.c:

	if (ce_mode_s_ifmt == S_IFREG) {
		struct stream_filter *filter = get_stream_filter(path, ce->sha1);
		if (filter &&
		    !streaming_write_entry(ce, path, filter,
					   state, to_tempfile,
					   &fstat_done, &st))
			goto finish;
	}

and get_stream_filter() in convert.c has an explicit exception for this
case at the very beginning:

struct stream_filter *get_stream_filter(const char *path, const unsigned char *sha1)
{
	struct conv_attrs ca;
	enum crlf_action crlf_action;
	struct stream_filter *filter = NULL;

	convert_attrs(&ca, path);

	if (ca.drv && (ca.drv->smudge || ca.drv->clean))
		return filter;

to make sure that it says "No streaming filtering is possible, do not even
attempt to call streaming_write_entry()".

^ permalink raw reply

* Re: [PATCH] index-pack: remove real_type from struct object_entry
From: Nguyen Thai Ngoc Duy @ 2012-02-24  6:08 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1330051320-19043-1-git-send-email-pclouds@gmail.com>

2012/2/24 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> @@ -581,7 +580,6 @@ static void resolve_delta(struct object_entry *delta_obj,
>  {
>        void *base_data, *delta_data;
>
> -       delta_obj->real_type = base->obj->real_type;
>        delta_obj->delta_depth = base->obj->delta_depth + 1;
>        if (deepest_delta < delta_obj->delta_depth)
>                deepest_delta = delta_obj->delta_depth;

This is wrong. Sorry for the noise.
-- 
Duy

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox