Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH v3] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script #1372

Closed
wants to merge 1 commit into from

Conversation

fobiasic07
Copy link
Contributor

@fobiasic07 fobiasic07 commented Oct 27, 2022

Changes since prevous 2 versions:

Replacing idiomatic helper functions

'! test_path_is_*'

with

'test_path_is_missing'

This uses values of 'test_path_bar' in place of '! test_path_foo' to
bring in the helpful factor of indicating the failure of tests after the
mv command has been used, that is, it echoes if the feature/test_path
exists .

Older test scripts use the command 'test -' to verify the presence or absence of features such as files, directories and symbolic links. This however requires slightly complicated uses, such as 'test ! -f '. The helper functions 'test_path_is_' located in t/test-lib-functions.sh have taken into account all scenarios of the 'test -' to reduce errors. This was a microproject to replace them with the helper functions.

Test script to verify the presence/absence of files, paths, directories,symboliclinks and many other features in mv command are using the command format

'test -(e|f|s|h|...).

Replace them with helper functions of format

'test_path_is_*'

Signed-off-by: Debra Obondo debraobondo@gmail.com
cc: Taylor Blau me@ttaylorr.com
cc: Martin Ågren martin.agren@gmail.com

@gitgitgadget-git
Copy link

Welcome to GitGitGadget

Hi @fobiasic07, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@gitgitgadget-git
Copy link

Invalid author email in 373cb44: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in 034c75d: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in 9d305c2: "97498997+fobiasic07@users.noreply.github.com"

@fobiasic07 fobiasic07 changed the title [PATCH] [OUTRACHY] t7001-mv.sh : Use test_path_is_* functions in test script [PATCH] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script Oct 27, 2022
t/t7001-mv.sh Outdated
git diff-files --quiet -- sub &&
git add .gitmodules &&
git mv sub mod/sub 2>actual.err &&
test_must_be_empty actual.err &&
! test -e sub &&
test_path_exists sub &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be test_path_is_missing sub instead, and I suspect that would fix the test failure, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, let me retry but would that be for all of the 'test -e' subs or the '! test -e'?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only be for ! test -e, the test -e alone tests for the existence and the ! inverses the condition.

@dscho
Copy link
Member

dscho commented Oct 27, 2022

/allow

@gitgitgadget-git
Copy link

User fobiasic07 is now allowed to use GitGitGadget.

WARNING: fobiasic07 has no public email address set on GitHub;
GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

@dscho
Copy link
Member

dscho commented Oct 27, 2022

@fobiasic07 my apologies, I meant to start off with a welcome (and while my feedback may sound as if I want to say that this patch is wrong, that is absolutely not the case, it's just that I often forget to first point out what is good about the diff, which is pretty much everything except for that minor bug). So please let me rectify that: welcome to the Git project, I hope you will have a good time, learn a lot, and make connections to people you like.

About this PR: please note that the Git project prefers the patch to be "polished", i.e. instead of adding commits that fix issues, please squash them into the original patch and then force-push.

Now I would like to talk about the commit message. The Git project puts an extraordinary emphasis on well-written commit messages, and since this is the first thing reviewers will read, it is a good idea to spend time to make it shine. The first line of the commit message, for example, is typically in the form ": ". See f01f948 as an example. In this instance, I would suggest "t7001: modernize file/directory existence tests". The commit message's body probably wants to provide a motivation for the change (here is some excellent advice what aspects excellent commit messages typically cover), I could imagine that the error messages provided by the test_path_* family of functions would be a good reason to modernize the script. Also, could you use your real name in the Signed-off-by: line instead of your GitHub handle?

I look forward to see you develop your patch!

Please do not hesitate to ask if anything is unclear or if you would like more assistance.

@fobiasic07
Copy link
Contributor Author

fobiasic07 commented Oct 27, 2022 via email

@nsengiyumva-wilberforce
Copy link
Contributor

I will make the edits and corrections and send it in. One question, once I'm done should I /submit directly?
@fobiasic07 , you are welcome here. I would not advise /submit immediately, it's better to first test your patch on your email alone by doing /preview to see if the patch will be successful

All the best,
Wilberforce

@gitgitgadget-git
Copy link

Invalid author email in 373cb44: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in 034c75d: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in 9d305c2: "97498997+fobiasic07@users.noreply.github.com"

@fobiasic07
Copy link
Contributor Author

fobiasic07 commented Oct 28, 2022

I will make the edits and corrections and send it in. One question, once I'm done should I /submit directly? @fobiasic07 , you are welcome here. I would not advise /submit immediately, it's better to first test your patch on your email alone by doing /preview to see if the patch will be successful

I'm doing it for my contribution, rather than editing and submitting it directly for me, you could suggest a few solutions. Thank you for the offer though. The patch has also passed the checks, it was a tiny bug. the only problem is squashing the commits before I submit it, for it to be 'polished' as required and recommended by both gitGuidelines and @dscho.

All the best, Wilberforce

Thanks, Debra Obondo

@fobiasic07
Copy link
Contributor Author

Hello @dscho

I look forward to see you develop your patch! Please do not hesitate to ask if anything is unclear or if you would like more assistance.

I'm having a challenge, some of the corrections I made to the code I did directly from the remote repo instead of from local and this is causing an issue with squashing the commits into one using the rebase interactive, do you have any recommendations for what I can do to solve this?

Thanks in advance, Debra Obondo.

@gitgitgadget-git
Copy link

Invalid author email in a00b908: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in f9d9b5d: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in a00b908: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in 4880fdd: "97498997+fobiasic07@users.noreply.github.com"

@gitgitgadget-git
Copy link

Invalid author email in a58c76a: "97498997+fobiasic07@users.noreply.github.com"

@fobiasic07
Copy link
Contributor Author

fobiasic07 commented Oct 28, 2022 via email

@fobiasic07
Copy link
Contributor Author

fobiasic07 commented Oct 28, 2022

Hello @dscho , So, I was able to squash 3/5 initial commits, a problem however occurred when I was trying to squash the already squashed commit with the remaining two and I ended up squashing previous (not authored by me) commits with my own commits and when I was trying to 'reset --HARD HEAD', I ended up doing a 'push --HARD' which closed the pull request and I need to reset it to my commits before I can open another PR, do you have any suggestions?

I was able to do undo the git reset, using the 'git reflog', so, no worries concerning the matter. I am yet to squash the three commits and submit a properly formatted commit and well corrected test file

@fobiasic07
Copy link
Contributor Author

On the Git mailing list, Taylor Blau wrote (reply to this):

Hi @ttaylorr

On Mon, Oct 31, 2022 at 07:04:20PM +0100, Martin Ågren wrote:
> Hi Debra,
>
> On Sun, 30 Oct 2022 at 18:35, Debra Obondo via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Debra Obondo <debraobondo@gmail.com>
> >
> > Test script to verify the presence/absence of files, paths, directories,
> > symlinks and other features in 'git mv' command are using the command
> > format:
> >
> > 'test (-e|f|d|h|...)'
> >
> > Replace them with helper functions of format:
> >
> > 'test_path_is_*'
>
> This is a good idea.
>
> The subject of this patch could use a space after the colon. You could
> also write "modernize" to give an order to the code base. So something
> like
>
>   t7001-mv.sh: modernize test script using function
>
> perhaps. "Function" is a bit vague, perhaps.
>
> I wanted to comment on this:
>
> >  test_expect_success 'mv --dry-run does not move file' '
> >         git mv -n path0/COPYING MOVED &&
> > -       test -f path0/COPYING &&
> > -       test ! -f MOVED
> > +       test_path_is_file path0/COPYING &&
> > +       ! test_path_is_file MOVED
> >  '
>
> It is my understanding that we prefer to only use such a helper when we
> really expect the file to exist. If the path is not a file, this helper
> prints a helpful message before returning with an error.
>
> Here, this means we will emit this 'helpful'
>
>   File MOVED doesn't exist
>
> on every test run, when really everything is as it should. And if the
> file is actually there, i.e., we have a bug, we'll emit nothing -- but
> that is precisely when we would want some diagnostics such as
>
>   Path exists:
>   ... MOVED ...
>
> to show us that the file actually exists, contrary to the test's
> expectations.
>
> Such output is precisely what `test_path_is_missing` would give us. :-)
>
> My gut feeling is that where this patch adds "! test_path_foo", it
> should use "test_path_bar" instead, for various values of "foo" and
> "bar". What do you think about that?

All good suggestions, thanks. I'll hold this back while we wait for a
rerolled version.

Question, according to the MyFirstContribution Documentation, it suggests that my patch should be submitted as just the first version if it is yet to be accepted into the 'next' stage, so, should I just squash my commits after making edits and correction and /submit it as it is or should I do a [PATCH V2] and will that be done using gitgitgadget-git or from my local?

Thanks,
Taylor

Thanks,
Debra

@fobiasic07 fobiasic07 requested a review from dscho November 2, 2022 20:34
@ttaylorr
Copy link
Member

ttaylorr commented Nov 3, 2022

should I just squash my commits after making edits and correction and /submit it as it is or should I do a [PATCH V2] and will that be done using gitgitgadget-git or from my local?

Hi @fobiasic07. You should squash any changes that you found during review and then submit the updated patch as a v2 in reply to the original one. Thanks!

@dscho
Copy link
Member

dscho commented Nov 3, 2022

@fobiasic07 please note that the Git project does not actually use Pull Requests, but instead all of the communication (including the patch contribution) is happening on the Git mailing list. GitGitGadget makes this a bit more convenient by offering to send the patches, and by mapping replies back to the PR as comments, but it does not send comments back out as mails.

Therefore, to reply to Martin and Taylor, please use the advice provided in https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis to reply directly, via mail.

@fobiasic07 fobiasic07 changed the title [PATCH] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script Nov 3, 2022
@fobiasic07
Copy link
Contributor Author

/preview

@gitgitgadget-git
Copy link

Preview email sent as pull.1372.v2.git.git.1667500484581.gitgitgadget@gmail.com

@fobiasic07
Copy link
Contributor Author

/submit

@gitgitgadget-git
Copy link

Submitted as pull.1372.v2.git.git.1667500788132.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1372/fobiasic07/t7001-v2

To fetch this version to local tag pr-git-1372/fobiasic07/t7001-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1372/fobiasic07/t7001-v2

@gitgitgadget-git
Copy link

On the Git mailing list, Martin Ågren wrote (reply to this):

On Thu, 3 Nov 2022 at 19:39, Debra Obondo via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Debra Obondo <debraobondo@gmail.com>
>
> Test script to verify the presence/absence of files, paths, directories,
> symlinks and other features in 'git mv' command are using the command
> format:
>
> 'test (-e|f|d|h|...)'
>
> Replace them with helper functions of format:
>
> 'test_path_is_*'

Ok.

> Changes since v1
>
> Replacing idiomatic helper functions
>
> '! test_path_is_*'
>
> with
>
> 'test_path_is_missing'
>

The above "Changes since v1" and what I've quoted here should probably
be dropped. We prefer our patches to look as if they've appeared out of
the blue in perfect shape. :-)

> This uses values of 'test_path_bar' in place of '! test_path_foo' to
> bring in the helpful factor of indicating the failure of tests after the
> mv command has been used, that is, it echoes if the feature/test_path
> exists.

This paragraph is excellent. It describes why you've done the patch this
way. This looks much better than version 1 of the patch!

> Signed-off-by: Debra Obondo <debraobondo@gmail.com>

After removing the lines about changes since v1, this patch is

Reviewed-by: Martin Ågren <martin.agren@gmail.com>

> ---
>     [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in
>     test script
>
>     Changes since v1:
>
>     Replacing idiomatic helper functions
>
>     '! test_path_is_*'
>
>     with
>
>     'test_path_is_missing'

This place after the "---" line is an excellent place for including such
"here's what has changed since v1" comments. Good. They will not appear
in the final commit message.

Thanks for contributing!

Martin

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 2f6ba16.

@fobiasic07 fobiasic07 changed the title [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script [PATCH v3] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test script Nov 4, 2022
Test script to verify the presence/absence of files, paths, directories,
symlinks and other features in 'git mv' command are using the command
format:

'test (-e|f|d|h|...)'

Replace them with helper functions of format:

'test_path_is_*'

Replacing idiomatic helper functions:

'! test_path_is_*'

with

'test_path_is_missing'

This uses values of 'test_path_bar' in place of '! test_path_foo' to
bring in the helpful factor of indicating the failure of tests after the
mv command has been used, that is, it echoes if the feature/test_path
exists.

Signed-off-by: Debra Obondo <debraobondo@gmail.com>
@fobiasic07
Copy link
Contributor Author

/submit

@gitgitgadget-git
Copy link

Submitted as pull.1372.v3.git.git.1667574352244.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1372/fobiasic07/t7001-v3

To fetch this version to local tag pr-git-1372/fobiasic07/t7001-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1372/fobiasic07/t7001-v3

@gitgitgadget-git
Copy link

On the Git mailing list, Taylor Blau wrote (reply to this):

Hi Debra,

On Fri, Nov 04, 2022 at 03:05:52PM +0000, Debra Obondo via GitGitGadget wrote:
>  t/t7001-mv.sh | 62 +++++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)

This round looks great. Will queue, thanks.

Thanks,
Taylor

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 0d16194.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via fdbeede.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 57b5007.

@gitgitgadget-git
Copy link

This patch series was integrated into next via dfc9c80.

@gitgitgadget-git gitgitgadget-git bot added the next label Nov 8, 2022
@gitgitgadget-git
Copy link

This patch series was integrated into seen via ff40b55.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 561f394.

@gitgitgadget-git
Copy link

This patch series was integrated into master via 561f394.

@gitgitgadget-git
Copy link

This patch series was integrated into next via 561f394.

@gitgitgadget-git
Copy link

Closed via 561f394.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants