public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: David Turner <dturner@twopensource•com>
Cc: Johannes Sixt <j6t@kdbg•org>,
	git@vger•kernel.org, David Turner <dturner@twitter•com>
Subject: Re: [PATCH v3 3/3] cat-file: add --follow-symlinks to --batch
Date: Mon, 11 May 2015 10:45:03 -0700	[thread overview]
Message-ID: <xmqqzj5bdn4w.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1431363819.15048.2.camel@ubuntu> (David Turner's message of "Mon, 11 May 2015 13:03:39 -0400")

David Turner <dturner@twopensource•com> writes:

> On Sun, 2015-05-10 at 09:06 +0200, Johannes Sixt wrote:
>> > +	ln -s loop2 loop1 &&
>> > +	git add . &&
>> > +	git commit -am "test"
>> > +'
>> 
>> These tests will fail on a file system that does not support symbolic 
>> links, such as on Windows. Would you please separate the test cases 
>> (that come after the setup) into two groups:
>> 
>> 1. Those that inspect the filesystem and expect a symbolic link. Protect 
>> these tests with a SYMLINKS prerequisite.
>
> I believe that none of these require that.
>
>> Note that you do not require a symlink enabled file system to generate a 
>> repository with symlinks, i.e., you don't have to protect the setup code 
>> with SYMLINKS. For this, you can use 'test_ln_s_add' instead of the 
>> above sequence of 'ln -s' followed by a single 'git add .'.
>
> Will fix, thanks.

Yeah, thanks. There doesn't seem to be any reason to forbid this new
feature from being used on symlink-challenged filesystem (after all,
"cat-file --batch" should run fine in a bare repository) and the
suggestion makes sens to me.

>> > +
>> > +echo $hello_sha1 blob $hello_size > found
>> 
>> This seems to be used only in the next test. Please move it inside 
>> test_expect_success.
>
> This is used in a number of tests, e.g this one:

Perhaps then the preparetion of 'found' is part of this thing:

    test_expect_success 'prep for symlink tests' '

A micronit on style; please drop SP between redirection and its
target (but have one before redirection), i.e.

	echo ... >found
	command <input

Thanks.

>
>> > +test_expect_success 'git cat-file --batch-check --follow-symlinks
>> > works for in-repo, same-dir links' '
>> > +	echo HEAD:same-dir-link | git cat-file --batch-check --follow-symlinks > actual &&
>> > +	test_cmp found actual
>> > +'

next prev parent reply	other threads:[~2015-05-11 17:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-09 20:36 [PATCH v3 1/3] tree-walk: learn get_tree_entry_follow_symlinks dturner
2015-05-09 20:36 ` [PATCH v3 2/3] sha1_name: get_sha1_with_context learns to follow symlinks dturner
2015-05-10 17:47   ` Junio C Hamano
2015-05-09 20:36 ` [PATCH v3 3/3] cat-file: add --follow-symlinks to --batch dturner
2015-05-10  7:06   ` Johannes Sixt
2015-05-11 17:03     ` David Turner
2015-05-11 17:45       ` Junio C Hamano [this message]
2015-05-10 17:42 ` [PATCH v3 1/3] tree-walk: learn get_tree_entry_follow_symlinks Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqzj5bdn4w.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=dturner@twitter$(echo .)com \
    --cc=dturner@twopensource$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=j6t@kdbg$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

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