Re: [PATCH] remote.<name>.pushurl does not consider aliases when pushing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Rob Hoelz <rob@xxxxxxxx> writes:

> Hi everyone!  I found a bug in Git today and wrote up a fix; I did my best to conform to the rules layed out in Documentation/SubmittingPatches, but please let me know if I need to change anything to get my work merged. =)  I have CC'ed Josh Triplet, as
> he was the last one to touch the line I modified.  I hope my commit messages explain the problem I encountered well enough; if not,
> I can always go back and amend them.
>
> Patches follow.
>
> -Rob


Please read Documentation/SubmittingPatches and follow it.  The
above is most likely to be the cover letter of a two-patch series
(meaning you will be sending three pieces of e-mail messages), or
perhaps out of band comment below the three-dash line of a single
patch (you will send only one piece of e-mail message).

See recent patches on the list from list regulars for good examples,
e.g.

    http://thread.gmane.org/gmane.comp.version-control.git/218350
    http://thread.gmane.org/gmane.comp.version-control.git/218177/focus=218361

> From 5007b11e86c0835807632cb41e6cfa75ce9a1aa1 Mon Sep 17 00:00:00 2001
> From: Rob Hoelz <rob@xxxxxxxx>
> Date: Sun, 17 Mar 2013 21:49:20 +0100
> Subject: [PATCH 1/2] Add test for pushInsteadOf + pushurl
>
> git push currently doesn't consider pushInsteadOf when
> using pushurl; this test tests that.
>
> Signed-off-by: Rob Hoelz <rob@xxxxxxxx>
> ---
>  t/t5500-push-pushurl.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 t/t5500-push-pushurl.sh

The number 5500 is already taken.  Please do not add a duplicate.

I also wonder if we need to waste a new test number for this;
perhaps adding new tests to 5516 that already tests insteadOf might
be a better fit, but I didn't carefully read it.

> diff --git a/t/t5500-push-pushurl.sh b/t/t5500-push-pushurl.sh
> new file mode 100644

Test scripts are supposed to be executable.

> +test_expect_success 'test commit and push' '
> +	test_commit one &&
> +	git push origin master:master
> +'
> +
> +test_expect_success 'check for commits in rw repo' '
> +	cd ../rw/repo &&
> +	git log --pretty=oneline | grep -q .
> +'

Are both expected to succeed in patch 1/2 without any code change?

If you were doing a large code change, it is a good series structure
to have tests first that are marked as "expect_failure" in an early
patch, and then in a later patch that changes the code to fix it,
update the tests that start to pass to "expect_success".

I personally do not think you need such a two-step approach for
something small like this; instead you can just have a single patch
that adds a set of tests that expect success and code change.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]