aur-fetch: unset GIT_DIR and GIT_WORK_TREE #636

tomeon · 2019-11-04T21:36:32Z

Using master at 1b4782e, aur-fetch exits with nonzero status under the following conditions:

It needs to fetch more than one repository
At least one repository (other than the first one listed on the command line) is being fetched for the first time (initial git clone):

$ cd "$(mktemp -d)"
$ aur fetch aurutils aurutils-git 
Cloning into 'aurutils'...                                         
remote: Enumerating objects: 348, done.                            
remote: Counting objects: 100% (348/348), done.                    
remote: Compressing objects: 100% (232/232), done.                 
remote: Total 348 (delta 117), reused 347 (delta 116)              
Receiving objects: 100% (348/348), 66.21 KiB | 622.00 KiB/s, done. 
Resolving deltas: 100% (117/117), done.                            
HEAD is now at 3872019 upgpkg: aurutils 2.3.2-1                    
From https://aur.archlinux.org/aurutils                            
 = [up to date]      master     -> origin/master                   
Already up to date.                                                
Current branch master is up to date.                               
fatal: working tree 'aurutils' already exists.                     
==> ERROR: fetch: aurutils-git: Failed to clone repository

The issue appears to be that aur-fetch doesn't unset GIT_DIR or GIT_WORK_TREE at the beginning of the "main" loop, thus reusing the values of GIT_DIR and GIT_WORK_TREE set on the prior iteration.

This merge request adds unset GIT_DIR GIT_WORK_TREE to the beginning of the main loop in aur-fetch, as well as a regression test case.

TIA for your consideration :)

AladW · 2019-11-04T23:02:51Z

This should be fixed along the way in 4dbab52

Anyway, a test case, nice. 👍

AladW · 2019-11-07T17:22:18Z

test/issue-636

+export AURDEST
+
+cleanup() {
+    rm -rf --one-file-system --preserve-root "${AURDEST?}"


A question since "cleanup" is used in other places with temporary files: why those extra flags to rm, if the target string is quoted?

Two things: an abundance of caution and (marginally) better diagnostics (well, three, including force of habit). Certainly not wedded to this approach, though, so I've harmonized this trap function with others in the existing codebase.

AladW · 2019-11-07T17:22:57Z

test/issue-636

@@ -0,0 +1,14 @@
+#!/bin/bash
+
+AURDEST="$(mktemp -d --tmpdir "aurutils-${BASH_SOURCE[0]##*/}.XXXXXXXX")"


I think it suffices to set argv0=issue-636, aurutils-$argv0 as other aur scripts

AladW · 2019-11-15T01:33:20Z

Should be fixed in master, but I'd still be interested in merging the regression test!

tomeon · 2019-11-20T01:38:01Z

Should be fixed in master

Indeed! I have rebased the branch on master, dropping my change to aur-fetch, and the regression test exits without error. So, barring false negatives... :)

but I'd still be interested in merging the regression test!

I've updated the PR in (what I hope is) accordance with your comments; LMKWYT.

AladW · 2019-11-21T17:45:59Z

Thanks for the update. Just a small nitpick: since aur-fetch does not actually use AURDEST, I'd remove it (and the export) and simply set $tmp like other scripts do.

rafasc

I seem to have missed this in my previous review.
The PR is not based on current master. Would be nice if you did that along with the suggestions from @AladW

to ensure that GIT_DIR/GIT_WORK_TREE environment variables are properly set on each iteration of the aur-fetch main loop

tomeon · 2019-11-21T20:04:55Z

Thanks for the update. Just a small nitpick: since aur-fetch does not actually use AURDEST, I'd remove it (and the export) and simply set $tmp like other scripts do.

Ah, yes -- suspect that was an artifact of an earlier test implementation that used aur-sync rather than calling aur-fetch directly. s/AURDEST/tmp/g done.

tomeon · 2019-11-21T20:06:53Z

I seem to have missed this in my previous review.
The PR is not based on current master. Would be nice if you did that along with the suggestions from @AladW

Just pushed a rebase on master; are you also looking for the test script to exercise some newly-added feature(s)?

AladW · 2019-11-21T20:25:33Z

It looks good to me. If we wanted to test more features, I'd say this belongs to other separate tests.

rafasc

LGTM 👍

rafasc · 2019-11-21T22:12:21Z

@tomeon tests for new features would be nice, but we need to address some aspects regarding tests in general. I.e. Implement proper infrastructure for testing to avoid issues like #645.
We have #356 suggesting TAP, but at some point it was also discussed using a simpler shell/makefile based suite.

Regardless of the approach chosen, we also need some sort of library with functions for common tasks, e.g. setting up repos, configs, etc. Making tests easier to write and run will, hopefully, help turning the #497 list into actual tests.

Speaking for myself here, but I rarely run the ones we have. Proper infrastructure, and perhaps automation (hello github actions), would help making sure we actually run the tests. Otherwise it becomes hard to justify the effort that needs to be put in.

tomeon · 2019-11-21T23:30:36Z

@rafasc -- sounds like a bigger discussion, and out-of-scope for this issue, but: what about BATS?

Seems to tick some of the boxes you listed:

Plus there's an available Docker image for use with (say) Travis CI, and an AUR package that could be used with (e.g.) builds.sr.ht.

Food for thought, perhaps :)

Issue #636

This passes the test in issue aurutils#636

AladW reviewed Nov 7, 2019

View reviewed changes

AladW added the regression-test label Nov 14, 2019

rafasc approved these changes Nov 21, 2019

View reviewed changes

rafasc requested changes Nov 21, 2019

View reviewed changes

test: add test of multi-package fetch

9cd148f

to ensure that GIT_DIR/GIT_WORK_TREE environment variables are properly set on each iteration of the aur-fetch main loop

tomeon requested a review from rafasc November 21, 2019 20:07

rafasc approved these changes Nov 21, 2019

View reviewed changes

AladW merged commit 9bf4208 into aurutils:master Nov 22, 2019

rafasc mentioned this pull request Nov 22, 2019

tests: TAP support #356

Closed

AladW added a commit that referenced this pull request Dec 31, 2019

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

03473b0

Issue #636

AladW mentioned this pull request Dec 31, 2019

aur-fetch: miscellaneous fixes #659

Closed

AladW added a commit that referenced this pull request Jan 16, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

70980a2

Issue #636

AladW added a commit that referenced this pull request Jan 16, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

596eff9

Issue #636

AladW added a commit that referenced this pull request Jan 20, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

edb55a8

Issue #636

AladW added a commit that referenced this pull request Jan 24, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

a802f80

Issue #636

AladW added a commit that referenced this pull request Jan 26, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

9d6593e

Issue #636

AladW added a commit that referenced this pull request Jan 26, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

b0fb55f

Issue #636

AladW added a commit that referenced this pull request Jan 26, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

140d886

Issue #636

AladW added a commit that referenced this pull request Jan 28, 2020

aur-fetch: correctly unset GIT_DIR, GIT_WORK_TREE

6d0a0d4

Issue #636

dhouck added a commit to dhouck/aurutils that referenced this pull request Feb 1, 2020

Fix aur-fetch of more than one package

d661cfd

This passes the test in issue aurutils#636

dhouck mentioned this pull request Feb 1, 2020

Fix aur-fetch of more than one package #675

Merged

dhouck added a commit to dhouck/aurutils that referenced this pull request Feb 1, 2020

Fix aur-fetch of more than one package

111a22a

This passes the test in issue aurutils#636

dhouck added a commit to dhouck/aurutils that referenced this pull request Feb 14, 2020

Fix aur-fetch of more than one package

d3abf7a

This passes the test in issue aurutils#636

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aur-fetch: unset GIT_DIR and GIT_WORK_TREE #636

aur-fetch: unset GIT_DIR and GIT_WORK_TREE #636

tomeon commented Nov 4, 2019

AladW commented Nov 4, 2019

AladW Nov 7, 2019

tomeon Nov 20, 2019

AladW Nov 7, 2019

tomeon Nov 20, 2019

AladW commented Nov 15, 2019

tomeon commented Nov 20, 2019

AladW commented Nov 21, 2019

rafasc left a comment

tomeon commented Nov 21, 2019

tomeon commented Nov 21, 2019

AladW commented Nov 21, 2019 •

edited

rafasc left a comment

rafasc commented Nov 21, 2019

tomeon commented Nov 21, 2019

		@@ -0,0 +1,14 @@
		#!/bin/bash

		AURDEST="$(mktemp -d --tmpdir "aurutils-${BASH_SOURCE[0]##*/}.XXXXXXXX")"

aur-fetch: unset GIT_DIR and GIT_WORK_TREE #636

aur-fetch: unset GIT_DIR and GIT_WORK_TREE #636

Conversation

tomeon commented Nov 4, 2019

AladW commented Nov 4, 2019

AladW Nov 7, 2019

Choose a reason for hiding this comment

tomeon Nov 20, 2019

Choose a reason for hiding this comment

AladW Nov 7, 2019

Choose a reason for hiding this comment

tomeon Nov 20, 2019

Choose a reason for hiding this comment

AladW commented Nov 15, 2019

tomeon commented Nov 20, 2019

AladW commented Nov 21, 2019

rafasc left a comment

Choose a reason for hiding this comment

tomeon commented Nov 21, 2019

tomeon commented Nov 21, 2019

AladW commented Nov 21, 2019 • edited

rafasc left a comment

Choose a reason for hiding this comment

rafasc commented Nov 21, 2019

tomeon commented Nov 21, 2019

AladW commented Nov 21, 2019 •

edited