git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees
Date: Mon, 12 Oct 2020 09:12:56 -0700	[thread overview]
Message-ID: <xmqqy2kbqv7b.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2010111221350.50@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Sun, 11 Oct 2020 12:28:33 +0200 (CEST)")

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Also, the patch specifically adjusts the GitHub workflow itself.
> Therefore, unlike the `skip_good_tree()` function, it does not pretend to
> be generic (which `skip_good_tree()` really is not, as pointed out above).

I think skip_good_tree aspired to be a generic one, by having the
"if we are not in travis nor GitHub actions, return early" at its
very beginning.  The person who adds support to GitHub workflow
could have done one of two things.  

One is to recognise the aspiration, and restructure existing
skip_good_tree from

    skip_good_tree () {
	return if not travis and if not github actions

	bunch of code that happens to work only on travis
    }

to

    skip_good_tree () {
	if travis
	then
	    bunch of code that happens to work only on travis
	else if github actions
	    bunch of code that happens to work only with github
	fi
    }

without touching the caller.  That lets skip_good_tree to be
generic.

Another is to completely ignore that aspiration, maybe doing all
that inside the workflow script (which by definition works only with
github).

I think the latter (i.e. what the patch choose to do, which is not
to bother with the ci/*.sh scripts at all) would be cleaner in this
particular case, but then it would have made the intention more
clear if the conditional at the beginning of skip_good_tree() were
adjusted, perhaps?

  reply	other threads:[~2020-10-12 16:13 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 17:17 [PATCH] ci: fix GitHub workflow when on a tagged revision Johannes Schindelin via GitGitGadget
2020-04-24 20:50 ` Junio C Hamano
2020-04-24 21:12   ` Johannes Schindelin
2020-04-24 21:24     ` Junio C Hamano
2020-10-08 15:29 ` [PATCH v2 0/2] Do not skip tagged revisions in the GitHub workflow runs Johannes Schindelin via GitGitGadget
2020-10-08 15:29   ` [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees Johannes Schindelin via GitGitGadget
2020-10-09  7:29     ` SZEDER Gábor
2020-10-09 11:13       ` Johannes Schindelin
2020-10-10  7:25         ` SZEDER Gábor
2020-10-11 10:28           ` Johannes Schindelin
2020-10-12 16:12             ` Junio C Hamano [this message]
2020-10-12 18:57               ` Johannes Schindelin
2020-10-15 17:17                 ` Junio C Hamano
2020-10-15 19:39                   ` Johannes Schindelin
2020-10-08 15:29   ` [PATCH v2 2/2] ci: do not skip tagged revisions in GitHub workflows Johannes Schindelin via GitGitGadget
2020-10-08 21:11   ` [PATCH v2 0/2] Do not skip tagged revisions in the GitHub workflow runs 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=xmqqy2kbqv7b.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=szeder.dev@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).