All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Nick Townsend <nick.townsend@mac.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"René Scharfe" <l.s.r@web.de>,
	"Jens Lehmann" <Jens.Lehmann@web.de>,
	"Heiko Voigt" <hvoigt@hvoigt.net>
Subject: Re: [PATCH] Additional git-archive tests
Date: Thu, 05 Dec 2013 11:52:58 -0800	[thread overview]
Message-ID: <xmqqwqjj3wit.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CA9E9538-E39B-41CA-BB82-BDD8CF7A2E3F@mac.com> (Nick Townsend's message of "Wed, 04 Dec 2013 18:49:39 -0800")

Nick Townsend <nick.townsend@mac.com> writes:

> Interplay between paths specified in three ways now tested:
> * After a : in the tree-ish,
> * As a pathspec in the command,
> * By virtue of the current working directory
>
> Note that these tests are based on the behaviours
> as found in 1.8.5. They may not be intentional.
> They were developed to regression test enhancements
> made to parse_treeish_arg() in archive.c

In other words, are all these new tests expected to pass?

My cursory read of parse_treeish_arg() in archive.c is:

 - It reads the given object with get_sha1(), checking if it is a
   commit-ish or tree-ish to decide if it wants to add the pax
   header to record the commit object name;

 - It parses the tree object;

 - If run from a subdirectory, attempts to grab the "prefix"
   (i.e. the path to the current subdirectory---in the tests you
   added, they are all "a/") out of that tree object (it errors out
   if it can't); and then

 - It archives the tree object.

So I do not think it is expected to accept tree object names with
the HEAD:<path> style syntax, if the user expects a predictable and
consistent result.  The third step above attempts to make sure that
you name a tree-ish that corresponds to the top-level of the
project, i.e. with no <path>.

What seems to be supported are:

    cd a && git archive HEAD ;# archives HEAD:a tree
    cd a && git archive HEAD -- b ;# archives a/b/ part of HEAD:a as b/

Specifically, it appears that HEAD:./b, HEAD:b etc. are not designed
to work, at least to me.

I am not saying that these should _not_ work, but it is unclear what
it means to "work".  For example, what should this do?

    cd a && git archive HEAD:./b $pathspec

The extended SHA-1 expression HEAD:./b in the subdirectory a/ is
interpreted by get_sha1_with_context_1() to be the name of the tree
object at path "a/b" in the commit HEAD.  Further, you are giving a
pathspec while in a subdirectory a/ to the command.  What should
that pathspec be relative to?

In a normal Git command, the pathspec always is relative to the
current subdirectory, so, the way to learn about the tree object
a/b/c in the HEAD while in subdirectory a/ would be:

    cd a && git ls-tree HEAD b/c

But what should the command line for archive to grab HEAD:a/b/c be?
It feels wrong to say:

    cd a && git archive HEAD:./b b/c

and it also feels wrong to say

    cd a && git archive HEAD:./b c

No matter what we would do, we should behave consistently with this
case:

    treeish=$(git rev-parse HEAD:a/b) &&
    cd a &&
    git archive $treeish -- $pathspec

so "take the pathspec relative to the tree when the treeish was
given with '<treeish>:<path>' syntax, and otherwise treat it
relative to the cwd" is not a workable solution.

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Nick Townsend <nick.townsend@mac.com>
> ---
>  t/t5004-archive-corner-cases.sh | 71 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>
> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index 67f3b54..a81a836 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -113,4 +113,75 @@ test_expect_success 'archive empty subtree by direct pathspec' '
>  	check_dir extract sub
>  '
>  
> +test_expect_success 'setup - repository with subdirs' '
> +	mkdir -p a/b/c a/b/d &&
> +	echo af >a/af &&
> +	echo bf >a/b/bf &&
> +	echo cf >a/b/c/cf &&
> +	git add a &&
> +	git commit -m "commit 1" &&
> +	git tag -a -m "rev-1" rev-1
> +'
> +
> +test_expect_success 'archive subtree from root by treeish' '
> +	git archive --format=tar HEAD:a >atreeroot.tar &&
> +	make_dir extract &&
> +	"$TAR" xf atreeroot.tar -C extract &&
> +	check_dir extract af b b/bf b/c b/c/cf
> +'
> +
> +test_expect_success 'archive subtree from root with pathspec' '
> +	git archive --format=tar HEAD a >atreepath.tar &&
> +	make_dir extract &&
> +	"$TAR" xf atreepath.tar -C extract &&
> +	check_dir extract a a/af a/b a/b/bf a/b/c a/b/c/cf
> +'
> +
> +test_expect_success 'archive subtree from root by 2-level treeish' '
> +	git archive --format=tar HEAD:a/b >abtreeroot.tar &&
> +	make_dir extract &&
> +	"$TAR" xf abtreeroot.tar -C extract &&
> +	check_dir extract bf c c/cf
> +'
> +
> +test_expect_success 'archive subtree from subdir' '
> +	(
> +		cd a &&
> +		git archive --format=tar HEAD >../asubtree.tar
> +	) &&
> +	make_dir extract &&
> +	"$TAR" xf asubtree.tar -C extract &&
> +	check_dir extract af b b/bf b/c b/c/cf
> +'
> +
> +test_expect_success 'archive subtree from subdir with treeish' '
> +	(
> +		cd a &&
> +		git archive --format=tar HEAD:./b >../absubtree.tar
> +	) &&
> +	make_dir extract &&
> +	"$TAR" xf absubtree.tar -C extract &&
> +	check_dir extract bf c c/cf
> +'
> +
> +test_expect_success 'archive subtree from subdir with treeish and pathspec' '
> +	(
> +		cd a &&
> +		git archive --format=tar HEAD:./b c >../absubtree.tar
> +	) &&
> +	make_dir extract &&
> +	"$TAR" xf absubtree.tar -C extract &&
> +	check_dir extract c c/cf
> +'
> +
> +test_expect_success 'archive subtree from subdir with alt treeish' '
> +	(
> +		cd a &&
> +		git archive --format=tar HEAD:b >../abxsubtree.tar
> +	) &&
> +	make_dir extract &&
> +	"$TAR" xf abxsubtree.tar -C extract &&
> +	check_dir extract bf c c/cf
> +'
> +
>  test_done

  reply	other threads:[~2013-12-05 19:53 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-03  0:10 [PATCH] Improvements to git-archive tests and add_submodule_odb() Nick Townsend
2013-12-03  0:14 ` Nick Townsend
2013-12-03 17:52   ` Junio C Hamano
2013-12-03 18:18   ` Heiko Voigt
2013-12-03 18:39     ` Re* " Junio C Hamano
2013-12-03 20:42       ` Heiko Voigt
2013-12-03  0:16 ` Nick Townsend
2013-12-03  9:26   ` Eric Sunshine
2013-12-03 17:54     ` Junio C Hamano
2013-12-05  2:49       ` [PATCH] Additional git-archive tests Nick Townsend
2013-12-05 19:52         ` Junio C Hamano [this message]
2013-12-10  5:26           ` Nick Townsend
2013-12-10 18:48             ` 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=xmqqwqjj3wit.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=hvoigt@hvoigt.net \
    --cc=l.s.r@web.de \
    --cc=nick.townsend@mac.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.